Implement full math and cmath modules #7
Conversation
📝 WalkthroughWalkthroughAdds complete Rust implementations of math and cmath (many functions), a libm FFI refactor (new Changes
Sequence Diagram(s)sequenceDiagram
participant RustAPI as Rust API (math / cmath)
participant Mwrap as crate::m (safe wrappers)
participant FFI as crate::m_sys (extern C)
participant Libm as platform libm
participant Err as crate::err
RustAPI->>Mwrap: call math/cmath function (e.g., sin, exp, sqrt)
Mwrap->>FFI: invoke extern C symbol
FFI->>Libm: call into libm / platform runtime
Libm-->>FFI: return value (may set errno)
FFI-->>Mwrap: return scalar result
Mwrap->>Err: call get_errno/is_error to inspect errno
alt errno indicates domain/overflow
Err-->>Mwrap: map to Error::EDOM / Error::ERANGE
Mwrap-->>RustAPI: return Err(...)
else no errno
Mwrap-->>RustAPI: return Ok(value)
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Fix all issues with AI agents
In @.python-version:
- Line 1: The repository pins local Python to "3.14.2" in .python-version which
can drift from CI that uses the floating "3.14" with check-latest; either update
.python-version to "3.14" to match CI's floating patch updates, or change CI's
python-version to the exact "3.14.2" to lock both environments to the same
patch; pick one approach and make the matching change so local and CI Python
versions remain consistent.
In @README.md:
- Around line 14-20: The README uses mixed heading styles causing markdownlint
MD003 warnings; make heading styles consistent by converting the ATX headings
like "## Module Structure" and "## Usage" to setext style (e.g., "Module
Structure" followed by "-----------------" and "Usage" followed by "-----") or
alternatively update the project's markdownlint config to permit ATX headings;
pick one approach and apply it consistently to the "Module Structure", "Usage"
and any other headings in README.md so all headings follow the same style.
In @src/cmath/exponential.rs:
- Around line 110-114: The EDOM check inside the branch guarded by
z.im.is_finite() && z.im != 0.0 is unreachable because it tests
z.im.is_infinite(); remove the dead check (the if testing z.im.is_infinite() and
its Err(Error::EDOM) return) from that branch and ensure the existing EDOM logic
remains only in the else branch that handles z.im infinite; update the branch
around z.im.is_finite()/z.im != 0.0 so it directly returns Ok(r) without the
impossible EDOM test.
In @src/cmath/misc.rs:
- Around line 48-61: The EDOM check "if r != 0.0 && !r.is_nan() &&
phi.is_infinite() { return Err(Error::EDOM); }" is unreachable because the
surrounding branch is guarded by "phi.is_finite() && phi != 0.0"; move or
restore the EDOM check to a place where phi may be infinite (e.g., before the
"if r.is_infinite() && phi.is_finite()..." branch) so it runs when
phi.is_infinite(), or remove the redundant check inside the branch; update
references to r, phi and Error::EDOM accordingly and ensure Complex64::new
creation path remains unchanged.
In @src/cmath/trigonometric.rs:
- Around line 196-218: In the tanh special-case branch that handles
z.re.is_infinite() && z.im.is_finite(), remove the unreachable check that tests
z.im.is_infinite() && z.re.is_finite() (which can never be true there); keep the
existing EDOM error check in the other branch that uses TANH_SPECIAL_VALUES and
special_type for mixed infinities, and simply return the computed r in the
infinite-real branch.
- Around line 100-127: The special-case branch in cosh that handles non-finite
inputs contains an unreachable check: inside the infinite-x / finite-y branch
(where you already asserted z.im.is_finite() && z.im != 0.0) you test
z.im.is_infinite() and set Error::EDOM, which can never be true; remove that
dead check from the first branch, and instead ensure the EDOM check for
z.im.is_infinite() is present only where it makes sense (e.g., the else branch
that uses COSH_SPECIAL_VALUES or by restructuring the outer conditionals),
updating references to z.re, z.im, special_type and Error::EDOM accordingly so
no unreachable code remains.
In @src/math/integer.rs:
- Line 764: The current test uses a tautology assert_eq!(comb(100, 50),
comb(100, 50)); — replace it with a real symmetry check by asserting comb(n, k)
== comb(n, n - k) for one or more values (e.g., assert_eq!(comb(100, 50),
comb(100, 50)); should be changed to something like assert_eq!(comb(100, 30),
comb(100, 70)) and add another case like assert_eq!(comb(101, 0), comb(101,
101)) or compare against a known exact value for comb(100, 50) if desired)
referencing the comb function so the test actually verifies symmetry.
🧹 Nitpick comments (17)
README.md (1)
22-29: Usage snippet is fine; consider showing non-panicking error handling if these returnResult.
Not required, but a short?example tends to mirror real usage better thanunwrap()in docs..github/workflows/rust.yml (2)
40-49: Feature-matrix intent: bigint runs likely include default features (and may miss--no-default-featurescoverage).
If you want to exercise bigint without the defaultcomplexfeature, add explicit--no-default-featuresruns; otherwise this is fine but worth making intentional (maybe via a matrix to keep runtime manageable).
27-32: Pin Python to exact patch version (e.g., "3.14.2") or removecheck-latest: truefor reproducible CI.With
check-latest: true, the workflow will automatically download newer 3.14.x patch releases when available, causing CI behavior to shift unpredictably. Usepython-version: "3.14.2"andcheck-latest: false(or omit it) for deterministic builds, or adopt automated tooling (e.g., Dependabot) to intentionally update the pinned version.Cargo.toml (1)
9-15: Double-check thatdefault = ["complex"]is intentional (dependency surface / compile time).
If the crate is meant to be “math-only by default”, you may wantdefault = []and let users opt intocomplex.src/math/integer.rs (2)
153-162: Prefer using the built-incount_ones()method.Rust's
u64::count_ones()compiles to a singlePOPCNTinstruction on most platforms, which is more efficient than the loop-based implementation.♻️ Suggested refactor
-/// Count the number of set bits in n -#[inline] -fn count_set_bits(mut n: u64) -> u64 { - let mut count = 0; - while n != 0 { - count += 1; - n &= n - 1; // clear least significant bit - } - count -} +/// Count the number of set bits in n +#[inline] +fn count_set_bits(n: u64) -> u64 { + n.count_ones() as u64 +}
633-656: Dead code reserved for future use.The
perm_combfunction is marked#[allow(dead_code)]and documented as reserved for futureBigUintn support. Consider adding a tracking issue or TODO with more context about when this will be enabled.src/math/misc.rs (1)
40-58: Consider validating tolerance parameters.The function doesn't validate that
rel_tol >= 0andabs_tol >= 0. Python'smath.iscloseraisesValueErrorfor negative tolerances. If this is intended to match Python semantics, consider adding validation.src/math/gamma.rs (1)
138-138: Unusual tuple extraction pattern.The pattern
Err((f64::NAN, Error::EDOM).1)creates a tuple and immediately extracts element.1. This is equivalent toErr(Error::EDOM). If thef64::NANvalue was intended for logging or documentation purposes, consider using a comment instead.♻️ Suggested simplification
- return Err((f64::NAN, Error::EDOM).1); + return Err(Error::EDOM); // would return NaNsrc/err.rs (2)
26-41: Platform coverage is incomplete for non-Linux/macOS Unix systems.The
set_errnofunction only handles Linux, macOS, and Windows, but will fail to compile on other Unix-like systems (FreeBSD, NetBSD, OpenBSD, etc.) due to missingcfgbranches. Consider adding a fallback using the POSIX-standard approach or a catch-all Unix branch.♻️ Suggested fix to handle other Unix platforms
unsafe { #[cfg(target_os = "linux")] { *libc::__errno_location() = value; } #[cfg(target_os = "macos")] { *libc::__error() = value; } #[cfg(target_os = "windows")] { libc::_set_errno(value); } + #[cfg(not(any(target_os = "linux", target_os = "macos", target_os = "windows")))] + { + // Most other Unix-like systems also use __errno_location or similar + compile_error!("errno support not implemented for this platform"); + } }
64-80: Verify the underflow threshold constant1.5against CPython.The magic number
1.5for distinguishing underflow (acceptable) from overflow (error) inERANGEhandling appears to come from CPython's implementation. Consider adding a comment referencing the source (e.g., CPython'smathmodule.c) to document this design decision.📝 Suggested documentation
+/// Check errno after libm call and convert to Result. +/// +/// The threshold of 1.5 for distinguishing underflow from overflow +/// matches CPython's mathmodule.c behavior. #[inline] pub(crate) fn is_error(x: f64) -> Result<f64> {src/cmath.rs (1)
14-18: Import organization can be improved for clarity.The
use crate::m;on line 16 is placed between two#[cfg(test)]blocks, which may cause confusion about whether it's test-only. Since it's used by submodules (not gated by test), consider grouping it with other non-test imports at the top.♻️ Suggested reorganization
+use crate::m; + #[cfg(test)] use crate::Result; -use crate::m; #[cfg(test)] use num_complex::Complex64;src/lib.rs (2)
19-26: Consider using compile-time#[cfg]instead of runtimecfg!check.While the compiler will likely optimize away the dead branch, using
#[cfg]directly provides clearer intent and guarantees compile-time dispatch. This mirrors the pattern used insrc/math/aggregate.rsfordl_mul.♻️ Suggested refactor
-#[inline(always)] -pub(crate) fn mul_add(a: f64, b: f64, c: f64) -> f64 { - if cfg!(feature = "mul_add") { - a.mul_add(b, c) - } else { - a * b + c - } -} +#[cfg(feature = "mul_add")] +#[inline(always)] +pub(crate) fn mul_add(a: f64, b: f64, c: f64) -> f64 { + a.mul_add(b, c) +} + +#[cfg(not(feature = "mul_add"))] +#[inline(always)] +pub(crate) fn mul_add(a: f64, b: f64, c: f64) -> f64 { + a * b + c +}
1-4: Consider consolidating thec()helper function.Based on the relevant code snippets, the
c(re: f64, im: f64) -> Complex64helper is duplicated insrc/cmath/exponential.rs,src/cmath/misc.rs, andsrc/cmath/trigonometric.rs. Consider defining it once in the parentcmathmodule and importing viasuper::c.src/math/aggregate.rs (1)
287-291: Allocation in subnormal handling path could be avoided.The recursive call creates a new
Vecfor scaled values. While this only triggers for subnormal inputs (rare), an in-place scaling approach would be more efficient if performance in edge cases matters.src/test.rs (1)
57-61: Simplify extraction:.ok().expect()pattern loses error context.The
.extract().ok().expect("failed to extract")pattern discards the actual extraction error. Using.extract().expect("...")directly would preserve the error message for better debugging.♻️ Suggested simplification
Ok(py_v) => { - let py_v: f64 = py_v.extract().ok().expect("failed to extract"); + let py_v: f64 = py_v.extract().expect("failed to extract f64 from Python result"); Some((py_v, v.unwrap())) }src/math.rs (1)
92-103: Consider documenting whyvector_norm_2is preferred overm::hypot.The implementation uses
aggregate::vector_norm_2instead of the libmhypot. While this likely provides better precision or edge-case handling, a brief comment explaining the rationale would help maintainability.📝 Suggested documentation addition
/// Return the Euclidean distance, sqrt(x*x + y*y). /// -/// Uses high-precision vector_norm algorithm +/// Uses high-precision vector_norm algorithm for better numerical stability +/// and consistent handling of edge cases (inf, nan) compared to libm hypot. #[inline] pub fn hypot(x: f64, y: f64) -> f64 {src/m_sys.rs (1)
69-97: Commented code for future f128 and f32 variants.The commented declarations for f128 types and conditional f32 variants indicate planned future work. Consider using a tracking issue or TODO comment to document when these should be enabled.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
.github/workflows/rust.yml.python-versionCargo.tomlREADME.mdproptest-regressions/cmath.txtproptest-regressions/gamma.txtproptest-regressions/math.txtproptest-regressions/math/exponential.txtsrc/cmath.rssrc/cmath/exponential.rssrc/cmath/misc.rssrc/cmath/trigonometric.rssrc/err.rssrc/lib.rssrc/m.rssrc/m_sys.rssrc/math.rssrc/math/aggregate.rssrc/math/exponential.rssrc/math/gamma.rssrc/math/integer.rssrc/math/misc.rssrc/math/trigonometric.rssrc/test.rs
💤 Files with no reviewable changes (1)
- proptest-regressions/gamma.txt
🧰 Additional context used
🧬 Code graph analysis (12)
src/math/trigonometric.rs (3)
src/m.rs (13)
sin(33-35)cos(28-30)tan(38-40)asin(13-15)acos(8-10)atan(18-20)atan2(23-25)sinh(65-67)cosh(60-62)tanh(70-72)asinh(50-52)acosh(45-47)atanh(55-57)src/math.rs (2)
math_1(52-65)math_2(71-80)src/test.rs (2)
test_math_1(76-95)test_math_2(98-147)
src/math/gamma.rs (3)
src/lib.rs (1)
mul_add(20-26)src/m.rs (2)
erf(198-200)erfc(203-205)src/math.rs (1)
math_1a(86-90)
src/math/exponential.rs (2)
src/m.rs (11)
exp(77-79)exp2(82-84)expm1(87-89)log(92-94)log10(97-99)log2(107-109)log1p(102-104)cbrt(114-116)pow(124-126)fmod(161-163)copysign(141-143)src/math.rs (1)
math_1(52-65)
src/cmath/misc.rs (2)
src/cmath.rs (1)
special_type(64-84)src/m.rs (2)
atan2(23-25)copysign(141-143)
src/math/misc.rs (3)
src/m_sys.rs (12)
ceil(44-44)floor(49-49)trunc(56-56)nextafter(54-54)fabs(46-46)copysign(45-45)frexp(51-51)exp(23-23)ldexp(52-52)modf(53-53)fmod(50-50)remainder(55-55)src/math.rs (2)
math_1(52-65)math_2(71-80)src/test.rs (2)
test_math_2(98-147)test_math_1(76-95)
src/math.rs (4)
src/math/aggregate.rs (2)
dist(329-355)fsum(106-194)src/m.rs (15)
cbrt(114-116)exp(77-79)exp2(82-84)expm1(87-89)log(92-94)log1p(102-104)log2(107-109)log10(97-99)pow(124-126)sqrt(129-131)erf(198-200)trunc(191-193)acos(8-10)cos(28-30)tan(38-40)src/err.rs (2)
set_errno(26-41)is_error(66-80)src/test.rs (2)
unwrap(52-73)test_math_2(98-147)
src/lib.rs (3)
src/cmath/exponential.rs (1)
c(18-20)src/cmath/misc.rs (1)
c(8-10)src/cmath/trigonometric.rs (1)
c(15-17)
src/cmath.rs (6)
src/cmath/exponential.rs (5)
exp(95-141)log(145-177)log10(181-184)sqrt(62-91)c(18-20)src/m_sys.rs (17)
exp(23-23)log(27-27)log10(28-28)sqrt(41-41)acos(6-6)acosh(15-15)asin(7-7)asinh(16-16)atan(8-8)atanh(17-17)cos(10-10)cosh(18-18)sin(11-11)sinh(19-19)tan(12-12)tanh(20-20)copysign(45-45)src/cmath/misc.rs (10)
abs(98-100)isclose(104-119)isfinite(80-82)isinf(92-94)isnan(86-88)phase(25-27)polar(31-38)rect(42-76)result(239-239)c(8-10)src/math/misc.rs (5)
isclose(46-58)isfinite(24-26)isinf(30-32)isnan(36-38)copysign(16-18)src/cmath/trigonometric.rs (13)
acos(347-369)acosh(292-307)asin(374-377)asinh(266-288)atan(382-385)atanh(311-343)cos(241-244)cosh(100-146)sin(249-253)sinh(150-194)tan(258-262)tanh(198-236)c(15-17)src/math/aggregate.rs (2)
e(427-427)e(435-435)
src/test.rs (1)
src/math/aggregate.rs (2)
e(427-427)e(435-435)
src/m.rs (1)
src/m_sys.rs (37)
acos(6-6)asin(7-7)atan(8-8)atan2(9-9)cos(10-10)sin(11-11)tan(12-12)acosh(15-15)asinh(16-16)atanh(17-17)cosh(18-18)sinh(19-19)tanh(20-20)exp(23-23)exp2(24-24)expm1(25-25)log(27-27)log10(28-28)log1p(29-29)log2(31-31)cbrt(34-34)hypot(37-37)pow(40-40)sqrt(41-41)ceil(44-44)copysign(45-45)fabs(46-46)floor(49-49)fmod(50-50)frexp(51-51)ldexp(52-52)modf(53-53)nextafter(54-54)remainder(55-55)trunc(56-56)erf(59-59)erfc(60-60)
src/cmath/trigonometric.rs (6)
src/m.rs (4)
sqrt(129-131)copysign(141-143)fabs(146-148)hypot(119-121)src/m_sys.rs (4)
sqrt(41-41)copysign(45-45)fabs(46-46)hypot(37-37)src/lib.rs (1)
mul_add(20-26)src/cmath/misc.rs (1)
c(8-10)src/math/misc.rs (2)
copysign(16-18)fabs(10-12)src/math.rs (1)
hypot(96-103)
src/m_sys.rs (1)
src/m.rs (37)
acos(8-10)asin(13-15)atan(18-20)atan2(23-25)cos(28-30)sin(33-35)tan(38-40)acosh(45-47)asinh(50-52)atanh(55-57)cosh(60-62)sinh(65-67)tanh(70-72)exp(77-79)exp2(82-84)expm1(87-89)log(92-94)log10(97-99)log1p(102-104)log2(107-109)cbrt(114-116)hypot(119-121)pow(124-126)sqrt(129-131)ceil(136-138)copysign(141-143)fabs(146-148)floor(156-158)fmod(161-163)frexp(166-168)ldexp(171-173)modf(176-178)nextafter(181-183)remainder(186-188)trunc(191-193)erf(198-200)erfc(203-205)
🪛 markdownlint-cli2 (0.18.1)
README.md
14-14: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
20-20: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
🔇 Additional comments (54)
README.md (1)
14-19: Docs: verify module paths + feature gating match the actual public API.
pymath::cmathis documented as feature-gated; ensurepymath::mis also actually public (and namedm, not e.g.m_sys) so the README doesn’t drift.proptest-regressions/math/exponential.txt (1)
1-9: LGTM; verify the regression file path matches the proptest module/test path so seeds are actually re-run.
If the test module name/path differs, proptest won’t pick upproptest-regressions/math/exponential.txt.Cargo.toml (1)
24-34:pyo3 0.27withauto-initializeis appropriate for dev-dependencies. Theauto-initializefeature automatically initializes the Python interpreter when acquiring the GIL (viaPython::with_gil()oracquire_gil()), which is suitable for test contexts and eliminates the need for manual interpreter setup. No breaking changes between 0.24 and 0.27 affect this feature.proptest-regressions/math.txt (1)
7-7: LGTM!The updated regression seed captures edge cases with subnormal floating-point values, which is valuable for testing numerical precision near the limits of IEEE 754 representation.
src/math/integer.rs (4)
14-31: LGTM!The
gcdandlcmimplementations correctly leveragenum_integerand handle edge cases (zero inputs, negative values) appropriately. Computinglcmas(a / gcd) * binstead of(a * b) / gcdavoids intermediate overflow.
54-124: LGTM!The
isqrtimplementation follows the adaptive-precision Newton iteration algorithm correctly. The fast path for values fitting inu64and the slow path for largerBigUintare well-structured. The final correction at line 119-121 ensures the exact floor of the square root is returned.
219-233: LGTM!The factorial implementation using the divide-and-conquer algorithm with odd-part factorization is efficient for large values. The lookup table optimization for small values and the binary split approach are well-implemented.
658-701: LGTM!The
combandpermfunctions have clean public APIs with proper edge case handling (k > n returning zero, symmetry optimization for combinations).src/math/exponential.rs (5)
5-23: LGTM!The exponential functions correctly delegate to
math_1withcan_overflow=true, ensuring overflow conditions returnERANGEas expected.
93-123: LGTM!The
logfunction with optional base correctly handles domain errors for both the argument and base, including the special case oflog(x, 1)which would cause division by zero.
174-236: LGTM!The
powfunction has comprehensive IEEE 754 special-case handling that matches CPython semantics. The distinction betweenEDOM(for0**negativeand negative base with non-integer exponent) andERANGE(for overflow) is correctly implemented.
238-406: LGTM!Comprehensive test coverage with both property-based tests for random inputs and edge tests for boundary conditions. The test harness comparing against Python's math module provides strong validation.
81-91: No action needed—edge case handling is correct.The current
m_log1pimplementation properly handles edge cases through the standard libmlog1pfunction combined with themath_1_fnwrapper validation. Whenlog1p(-1)returns-inforlog1p(x < -1)returnsNaN, the wrapper detects these as invalid (infinite or NaN for a finite input) and returnsEDOMerror, matching Python's behavior. This is consistent with IEEE 754 standards and is not platform-dependent. Unlikem_log, the edge validation happens at the wrapper level rather than inm_log1pitself, but the outcome is identical.Likely an incorrect or invalid review comment.
src/math/trigonometric.rs (3)
5-51: LGTM!The trigonometric functions correctly use
can_overflow=falsesince they have bounded outputs (sin/cos) or domain errors manifest as NaN rather than overflow (tan, inverse functions).
53-89: LGTM!The hyperbolic functions correctly distinguish between functions that can overflow (
sinh,cosh) and those that are bounded or have domain restrictions (tanh,asinh,acosh,atanh).
91-298: LGTM!Thorough test coverage for all trigonometric and hyperbolic functions with both edge case and property-based testing.
src/math/misc.rs (6)
60-72: LGTM!The
frexpfunction correctly handles special cases (NaN, infinity, zero) directly to ensure consistent behavior across platforms.
74-88: LGTM!The
ldexpfunction correctly handles special inputs and reportsERANGEfor overflow conditions.
90-106: LGTM!The
modffunction correctly handles special cases for infinity and NaN, matching Python's behavior.
108-122: LGTM!The
fmodfunction correctly handles the special case of infinite divisor and properly signals domain errors.
130-147: LGTM!The
ulpfunction correctly handles edge cases including the largest representable float where it computes the ULP by looking backward instead of forward.
149-434: LGTM!Comprehensive test coverage with PyO3-based comparison against Python's math module, including both deterministic edge tests and property-based testing.
proptest-regressions/cmath.txt (1)
1-10: LGTM!The proptest regression seeds capture valuable edge cases with extreme exponent combinations for complex number operations, which will help ensure numerical stability across the cmath module.
src/math/gamma.rs (3)
6-16: LGTM!The
erfanderfcfunctions correctly delegate to the underlying libm implementations with proper error handling viamath_1a.
21-26: LGTM!The
#[allow(clippy::excessive_precision)]annotations are appropriate for the Lanczos coefficients, which require high precision for accurate gamma function computation.
271-279: LGTM!The test macro invocations for
erfanderfcfollow the established pattern and ensure comprehensive test coverage alongside the existinggammaandlgammatests.src/cmath.rs (1)
86-120: LGTM!The
assert_complex_eqfunction provides thorough comparison logic with proper handling of NaN, infinity (including sign matching), and finite values with exact bit comparison. The strict ULP check ensures CPython compatibility.src/cmath/exponential.rs (2)
61-91: LGTM!The
sqrtimplementation correctly handles special values via the lookup table, zeros (preserving sign of imaginary part), subnormal inputs (with proper scaling), and uses the standard formula with symmetry reduction. The algorithm matches CPython'scmathmodule.c.
143-177: LGTM!The
logimplementation properly handles:
- Special values via lookup table
- Large magnitudes (scaled to avoid overflow)
- Subnormal magnitudes (scaled up for precision)
- Normal magnitudes with a precision-preserving
log1ppath for values near unity (0.71-1.73 range)- Zero input returning EDOM error
src/cmath/misc.rs (2)
102-119: LGTM!The
iscloseimplementation correctly handles edge cases: exact equality short-circuit (which handles matching infinities), rejection of mismatched infinities or inf-finite pairs, and standard relative/absolute tolerance comparison for finite values.
121-351: LGTM!The test module provides comprehensive coverage:
- Edge tests covering all combinations of
EDGE_VALUES- Property-based tests using proptest for random inputs
- Direct comparison against CPython's cmath module
- Proper NaN handling in comparisons
src/math/aggregate.rs (2)
106-194: LGTM!The
fsumimplementation correctly follows Shewchuk's algorithm for accurate floating-point summation:
- Maintains a vector of non-overlapping partial sums
- Properly handles intermediate overflow (ERANGE)
- Correctly detects
-inf + infdomain error (EDOM)- Implements half-even rounding correction for the final result
357-377: LGTM!The
sumprodimplementation uses TripleLength arithmetic for high precision accumulation and correctly falls back to simple multiplication when encountering overflow or special values, matching Python'smath.sumprodbehavior.src/test.rs (2)
4-47: LGTM!The
EDGE_VALUESarray provides comprehensive coverage of floating-point edge cases including:
- Signed zeros and infinities
- Multiple NaN representations (including payload variant)
- Subnormal values
- Boundary values (
MIN_POSITIVE,MAX,MIN)- Various magnitudes and trigonometric special values
75-147: LGTM!The
test_math_1andtest_math_2helpers provide a clean abstraction for testing Rust math functions against Python's math module:
- Proper NaN handling (both NaN = pass)
- Correct exception mapping (ValueError→EDOM, OverflowError→ERANGE)
- Bit-exact comparison for non-NaN values
- Clear error messages on mismatch
src/math.rs (5)
1-23: Well-organized module structure and re-exports.The module hierarchy is clean with clear separation of concerns. The re-exports provide a comprehensive Python-like math surface. The conditional
_bigintfeature gate for the integer module is appropriate.
24-46: Clean macro for simple libm wrappers.The
libm_simplemacro effectively reduces boilerplate for generating pass-through wrappers. Good use of#[inline]for performance.
48-90: Error handling wrappers are well-designed.The
math_1,math_2, andmath_1afunctions implement appropriate error detection:
math_1distinguishes between overflow (ERANGE) and domain errors (EDOM) viacan_overflowparametermath_2assumes all overflow cases are range errorsmath_1adelegates to errno-based detectionThis matches Python's math module behavior for error reporting.
105-134: Constants and angle conversions are correct.The constants properly reference
std::f64::constsand the angle conversion formulas are standard. The#[inline]hints are appropriate for these trivial functions.
136-233: Comprehensive test coverage with Python parity checks.Good use of edge value testing and property-based testing via proptest. The comparison against Python's math module via pyo3 ensures behavioral compatibility.
src/m.rs (3)
1-41: Clean wrapper layer for trigonometric functions.The consistent pattern of
#[inline(always)]wrappers around unsafe m_sys calls provides a clean abstraction. The module comment accurately describes the purpose.
42-132: Hyperbolic, exponential, logarithmic, and power function wrappers look good.All wrappers follow the established pattern and correctly delegate to the corresponding m_sys functions.
133-193: Safe wrappers for floating-point manipulation functions.The
frexpandmodfwrappers correctly accept&mutreferences, which safely coerce to raw pointers when passed to the underlying FFI calls. The commented-outfdimsuggests intentional exclusion.src/cmath/trigonometric.rs (7)
1-17: Imports and constants are well-organized.The local constants
CM_SQRT_LARGE_DOUBLEandCM_SQRT_DBL_MINare appropriately defined with documented derivations. Thechelper for Complex64 construction improves readability.
19-96: Special value tables implement Python cmath edge-case behavior.The 7x7 lookup tables cover all combinations of special floating-point values (negative inf, negative finite non-zero, negative zero, positive zero, positive finite non-zero, positive inf, NaN). This aligns with CPython's cmath implementation.
219-236: Correct handling of large real parts intanh.The large-magnitude optimization avoids overflow in
2.0 * z.imby using the exponential decay form. The formula is mathematically correct.
238-262: Correct identity-based implementations for cos, sin, tan.The implementations correctly use:
cos(z) = cosh(iz)sin(z) = -i * sinh(iz)tan(z) = -i * tanh(iz)The result transformation
(s.im, -s.re)correctly extracts the-i * soperation.
264-343: Robust implementations of inverse hyperbolic functions.The
asinh,acosh, andatanhfunctions include:
- Special value table lookups via
special_value!macro- Large-argument optimizations to avoid overflow
- Numerically stable formulations using
sqrt,log,hypotThe reduction for
atanhwhenz.re < 0uses the symmetry property correctly.
345-385: Correct inverse trigonometric implementations.The
acosfunction has a direct implementation with large-argument handling. Theasinandatanfunctions correctly use identities:
asin(z) = -i * asinh(iz)atan(z) = -i * atanh(iz)
387-605: Comprehensive test coverage for complex trigonometric functions.Good coverage with:
- Edge value tests covering all special floating-point combinations
- Property-based tests via proptest for random inputs
- Delegation to shared test harness for Python comparison
src/m_sys.rs (4)
1-12: FFI bindings module for system libm.The module header and initial declarations are clean. The
#[allow(dead_code)]is appropriate since not all bindings may be used immediately.
36-39: Correct platform-specific link names for MSVC.The
_hypotand_hypotflink names are the correct MSVC-specific symbol names for these functions.
63-65: AIX-specific exclusion forlgammaf_ris appropriate.The
lgammaf_rfunction is known to be unavailable on AIX. The#[cfg(not(target_os = "aix"))]guard correctly handles this platform limitation.
58-67: f32 functions are declared but unused, not a linker concern.The f32 variants (
expm1f,log1pf,cbrtf,hypotf,fdimf,erff,erfcf,lgammaf_r,tgammaf) are declared inm_sys.rsfor API completeness, but no safe wrappers are provided inm.rssince the library focuses on f64 (matching Python'sfloattype). This is intentional design, not an issue—unused FFI declarations do not cause linker errors, and the#[allow(dead_code)]attribute appropriately suppresses warnings.Likely an incorrect or invalid review comment.
| ## Module Structure | ||
|
|
||
| - `pymath::math` - Real number math functions (Python's `math` module) | ||
| - `pymath::cmath` - Complex number functions (Python's `cmath` module, requires `complex` feature) | ||
| - `pymath::m` - Direct libm bindings | ||
|
|
||
| ## Usage |
There was a problem hiding this comment.
Resolve markdownlint MD003 heading-style warnings (consistency).
Either switch these headings to setext style or update the markdownlint config to accept ATX headings.
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
14-14: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
20-20: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
🤖 Prompt for AI Agents
In @README.md around lines 14 - 20, The README uses mixed heading styles causing
markdownlint MD003 warnings; make heading styles consistent by converting the
ATX headings like "## Module Structure" and "## Usage" to setext style (e.g.,
"Module Structure" followed by "-----------------" and "Usage" followed by
"-----") or alternatively update the project's markdownlint config to permit ATX
headings; pick one approach and apply it consistently to the "Module Structure",
"Usage" and any other headings in README.md so all headings follow the same
style.
| // Platform-specific sincos | ||
|
|
||
| /// Result type for sincos function (matches Apple's __double2) | ||
| #[cfg(target_os = "macos")] | ||
| #[repr(C)] | ||
| struct SinCosResult { | ||
| sin: f64, | ||
| cos: f64, | ||
| } | ||
|
|
||
| #[cfg(target_os = "macos")] | ||
| unsafe extern "C" { | ||
| pub fn acos(n: f64) -> f64; | ||
| pub fn asin(n: f64) -> f64; | ||
| pub fn atan(n: f64) -> f64; | ||
| pub fn atan2(a: f64, b: f64) -> f64; | ||
| pub fn cbrt(n: f64) -> f64; | ||
| pub fn cbrtf(n: f32) -> f32; | ||
| pub fn cosh(n: f64) -> f64; | ||
| pub fn expm1(n: f64) -> f64; | ||
| pub fn expm1f(n: f32) -> f32; | ||
| pub fn fdim(a: f64, b: f64) -> f64; | ||
| pub fn fdimf(a: f32, b: f32) -> f32; | ||
| #[cfg_attr(target_env = "msvc", link_name = "_hypot")] | ||
| pub fn hypot(x: f64, y: f64) -> f64; | ||
| #[cfg_attr(target_env = "msvc", link_name = "_hypotf")] | ||
| pub fn hypotf(x: f32, y: f32) -> f32; | ||
| pub fn log1p(n: f64) -> f64; | ||
| pub fn log1pf(n: f32) -> f32; | ||
| pub fn sinh(n: f64) -> f64; | ||
| pub fn tan(n: f64) -> f64; | ||
| pub fn tanh(n: f64) -> f64; | ||
| pub fn tgamma(n: f64) -> f64; | ||
| pub fn tgammaf(n: f32) -> f32; | ||
| pub fn lgamma_r(n: f64, s: &mut i32) -> f64; | ||
| #[cfg(not(target_os = "aix"))] | ||
| pub fn lgammaf_r(n: f32, s: &mut i32) -> f32; | ||
| pub fn erf(n: f64) -> f64; | ||
| pub fn erff(n: f32) -> f32; | ||
| pub fn erfc(n: f64) -> f64; | ||
| pub fn erfcf(n: f32) -> f32; | ||
|
|
||
| // pub fn acosf128(n: f128) -> f128; | ||
| // pub fn asinf128(n: f128) -> f128; | ||
| // pub fn atanf128(n: f128) -> f128; | ||
| // pub fn atan2f128(a: f128, b: f128) -> f128; | ||
| // pub fn cbrtf128(n: f128) -> f128; | ||
| // pub fn coshf128(n: f128) -> f128; | ||
| // pub fn expm1f128(n: f128) -> f128; | ||
| // pub fn hypotf128(x: f128, y: f128) -> f128; | ||
| // pub fn log1pf128(n: f128) -> f128; | ||
| // pub fn sinhf128(n: f128) -> f128; | ||
| // pub fn tanf128(n: f128) -> f128; | ||
| // pub fn tanhf128(n: f128) -> f128; | ||
| // pub fn tgammaf128(n: f128) -> f128; | ||
| // pub fn lgammaf128_r(n: f128, s: &mut i32) -> f128; | ||
| // pub fn erff128(n: f128) -> f128; | ||
| // pub fn erfcf128(n: f128) -> f128; | ||
|
|
||
| // cfg_if::cfg_if! { | ||
| // if #[cfg(not(all(target_os = "windows", target_env = "msvc", target_arch = "x86")))] { | ||
| // pub fn acosf(n: f32) -> f32; | ||
| // pub fn asinf(n: f32) -> f32; | ||
| // pub fn atan2f(a: f32, b: f32) -> f32; | ||
| // pub fn atanf(n: f32) -> f32; | ||
| // pub fn coshf(n: f32) -> f32; | ||
| // pub fn sinhf(n: f32) -> f32; | ||
| // pub fn tanf(n: f32) -> f32; | ||
| // pub fn tanhf(n: f32) -> f32; | ||
| // }} | ||
| #[link_name = "__sincos_stret"] | ||
| fn sincos_stret(x: f64) -> SinCosResult; | ||
| } | ||
|
|
||
| /// Compute sin and cos together using Apple's optimized sincos. | ||
| /// This matches Python's cmath behavior on macOS. | ||
| #[cfg(target_os = "macos")] | ||
| #[inline(always)] | ||
| pub fn sincos(x: f64) -> (f64, f64) { | ||
| let sc = unsafe { sincos_stret(x) }; | ||
| (sc.sin, sc.cos) | ||
| } | ||
|
|
||
| /// Fallback for non-macOS: call sin and cos separately | ||
| #[cfg(not(target_os = "macos"))] | ||
| #[inline(always)] | ||
| pub fn sincos(x: f64) -> (f64, f64) { | ||
| (sin(x), cos(x)) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
__sincos_stret macOS f64 sincos Apple availability
💡 Result:
Short answer: __sincos_stret is an Apple/Darwin runtime symbol (struct-returning sin+cos for double) provided by libsystem on macOS and has been present on macOS since about 10.9. LLVM/Clang can emit calls to it on Darwin targets; projects targeting older macOS must set an appropriate deployment target or avoid codegen that emits that symbol. [1][2][3]
Sources:
- Explanation and evidence that ___sincos_stret is in /usr/lib/system/libsystem_m.dylib and appeared on OS X ≈10.9 (and iOS ≈7.0). [1]
- LLVM change discussion showing Darwin-specific sincos/sincos_stret handling (LLVM emits/guards __sincos_stret). [2]
- GNU/Gnulib notes about sincos portability (macOS historically differs in availability of sincos variants). [3]
Platform-specific sincos implementation is well-designed, but add a minimum deployment target requirement.
The macOS path uses Apple's optimized __sincos_stret which is available in libsystem_m.dylib and returns both sin and cos values efficiently. The #[repr(C)] on SinCosResult ensures correct ABI compatibility.
__sincos_stret is available on macOS 10.9+ and iOS 7.0+. Ensure your project's deployment target is set to at least macOS 10.9. If supporting older versions is required, add version-specific guards or runtime checks.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @src/cmath/trigonometric.rs:
- Around line 530-590: The assertion routine assert_complex_eq (in src/cmath.rs)
currently panics whenever ulp_diff != 0 which enforces bit-exact equality;
change it to allow a small ULP tolerance (e.g., accept differences up to 2 ULPs)
by replacing the strict check with something like if ulp_diff > 2 then panic,
and update the failure message to include the actual ulp_diff and the tolerated
threshold so test output is informative; keep the rest of the comparison logic
intact so proptest-based trig tests (proptest_* in trigonometric.rs) can
tolerate tiny rounding differences.
In @src/math/integer.rs:
- Around line 1-13: Add a compile-time guard that errors if both bigint features
are enabled to avoid import collisions: insert a cfg check like
#[cfg(all(feature = "malachite-bigint", feature = "num-bigint"))] followed by a
compile_error! with a short message instructing users to enable only one bigint
feature; place this near the top of the integer module where BigInt/BigUint are
imported so the conflict is caught early. Also update CI to include a matrix job
that enables both features simultaneously to ensure the guard triggers in CI.
Ensure references to BigInt and BigUint (the imports guarded by cfg(feature =
"...")) remain unchanged aside from the added compile-time check.
🧹 Nitpick comments (5)
src/cmath/misc.rs (1)
95-110: isclose implementation differs from math.isclose - consider alignment.The complex
isclosefunction returnsbooldirectly and doesn't validate tolerances (lines 95-110), whereas the real-valued version insrc/math/misc.rslines 47-63 returnsResult<bool>and checks for negative tolerances. Consider whether complexiscloseshould also validate tolerances and returnResult<bool>for API consistency.src/m.rs (1)
7-205: Consistent wrapper pattern with aggressive inlining.All math function wrappers follow a consistent pattern of
unsafe { m_sys::function(...) }with#[inline(always)]. This aggressive inlining is appropriate for hot-path math functions, though it will increase code size. Consider whether all functions needalwaysvs. justinline.src/math/integer.rs (3)
59-63: Add a debug assertion forapproximate_isqrt’s required input range.The table index assumes
2**62 <= n < 2**64; a debug assertion would make misuse fail fast in debug builds.
159-184: Consider debug-asserting helper preconditions in factorial internals.
factorial_partial_productassumes “start/stop odd and stop > start”. Adebug_assert!would help prevent accidental misuse if these helpers are reused later.Also applies to: 186-228
628-651:perm_combis dead_code; consider either wiring it in or removing it to avoid bitrot.If BigUint-
nsupport is planned “soon”, maybe gate it behind a feature flag or add a minimal test to keep it from silently breaking.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
.github/workflows/rust.yml.python-versionCargo.tomlproptest-regressions/cmath.txtproptest-regressions/math.txtsrc/cmath.rssrc/cmath/exponential.rssrc/cmath/misc.rssrc/cmath/trigonometric.rssrc/err.rssrc/lib.rssrc/m.rssrc/m_sys.rssrc/math.rssrc/math/aggregate.rssrc/math/exponential.rssrc/math/integer.rssrc/math/misc.rs
🚧 Files skipped from review as they are similar to previous changes (7)
- src/cmath/exponential.rs
- src/math/aggregate.rs
- src/err.rs
- src/cmath.rs
- .python-version
- src/math.rs
- src/math/misc.rs
🧰 Additional context used
🧬 Code graph analysis (4)
src/lib.rs (1)
src/cmath.rs (1)
c(41-43)
src/cmath/trigonometric.rs (4)
src/cmath.rs (2)
c(41-43)special_type(70-90)src/lib.rs (1)
mul_add(20-26)src/math/misc.rs (2)
copysign(16-18)fabs(10-12)src/math.rs (1)
hypot(97-103)
src/cmath/misc.rs (3)
src/cmath.rs (2)
c(41-43)special_type(70-90)src/m.rs (7)
atan2(23-25)hypot(119-121)copysign(141-143)cos(28-30)sin(33-35)sincos(237-240)sincos(245-247)src/math/misc.rs (5)
copysign(16-18)isfinite(24-26)isnan(36-38)isinf(30-32)isclose(48-64)
src/m_sys.rs (3)
src/m.rs (37)
acos(8-10)asin(13-15)atan(18-20)atan2(23-25)cos(28-30)sin(33-35)tan(38-40)acosh(45-47)asinh(50-52)atanh(55-57)cosh(60-62)sinh(65-67)tanh(70-72)exp(77-79)exp2(82-84)expm1(87-89)log(92-94)log10(97-99)log1p(102-104)log2(107-109)cbrt(114-116)hypot(119-121)pow(124-126)sqrt(129-131)ceil(136-138)copysign(141-143)fabs(146-148)floor(156-158)fmod(161-163)frexp(166-168)ldexp(171-173)modf(176-178)nextafter(181-183)remainder(186-188)trunc(191-193)erf(198-200)erfc(203-205)src/math.rs (1)
hypot(97-103)src/math/misc.rs (7)
copysign(16-18)fabs(10-12)fmod(118-128)frexp(70-78)ldexp(84-94)modf(100-112)remainder(132-134)
🪛 GitHub Actions: Rust
src/math/exponential.rs
[error] 89-89: edgetest_cbrt test failed: assertion did not match expected value.
[error] 278-278: proptest_cbrt test failed: assertion did not match expected value.
src/cmath/trigonometric.rs
[error] 530-530: proptest_asinh failed: minimal failing input re=0.0396710..., im=9.85e-191 with ULP diff=1.
[error] 530-530: proptest_atan failed: minimal failing input re=3.14159..., im=6.28318... with ULP diff=1.
[error] 530-530: proptest_atanh failed: minimal failing input re=1e-10..., im=6.28318... with ULP diff=1.
[error] 530-530: proptest_tan failed: minimal failing input re≈-2.738e116, im≈-2.504e29 with ULP diff=1.
[error] 530-530: proptest_tanh failed: minimal failing input re≈-3.331e-206, im≈-2.504e29 with ULP diff=1.
src/cmath/misc.rs
[error] 128-128: OverflowError: math range error during edgetest_phase.
[error] 128-128: OverflowError: math range error during proptest_phase.
🔇 Additional comments (22)
.github/workflows/rust.yml (2)
41-48: Test coverage expansion looks good.The addition of feature-specific test runs (complex, num-bigint, malachite-bigint) properly validates the optional dependency matrix introduced in Cargo.toml.
30-30: Python 3.14 is officially released (October 7, 2025) and is available in GitHub Actions, including support foractions/setup-python@v5. No action required.proptest-regressions/cmath.txt (1)
1-10: LGTM!Proptest regression data properly formatted for reproducing known failure cases.
proptest-regressions/math.txt (1)
7-7: LGTM!Updated proptest regression data reflects new shrink targets from test runs.
Cargo.toml (2)
10-14: Feature structure is well-designed.The feature hierarchy with
_bigintas an internal feature and user-facingnum-bigintandmalachite-bigintfeatures provides good separation and flexibility.
34-34: No action needed—pyo3 version 0.27 is released and available.Version 0.27.0 was released in October 2025, with 0.27.2 being the latest patch release (November 30, 2025). The version is available on crates.io.
src/lib.rs (3)
1-4: Module structure looks good.Feature-gating
cmathunder thecomplexfeature and exposingmathpublicly aligns well with the feature matrix defined in Cargo.toml.
20-26: Use compile-time feature check instead of runtimecfg!macro.The
cfg!macro performs a runtime check, meaning both code paths are compiled into the binary. For feature flags, use the#[cfg]attribute for compile-time conditional compilation.🐛 Proposed fix using compile-time conditional compilation
#[inline(always)] pub(crate) fn mul_add(a: f64, b: f64, c: f64) -> f64 { - if cfg!(feature = "mul_add") { + #[cfg(feature = "mul_add")] + { a.mul_add(b, c) - } else { + } + #[cfg(not(feature = "mul_add"))] + { a * b + c } }Likely an incorrect or invalid review comment.
36-48: No action needed.Python::attachis a documented, stable API in pyo3 0.27 (as specified in the project's dev-dependencies). The code correctly uses this API with theauto-initializefeature enabled, which allows the interpreter to initialize if needed.src/math/exponential.rs (3)
176-236: Comprehensive IEEE 754 special-case handling in pow.The
powfunction implements extensive edge-case handling for NaN, infinities, and special exponent values, with proper EDOM/ERANGE error mapping. The logic correctly handles complex scenarios likeNaN**0 = 1,1**NaN = 1, and signed zero results.
93-123: Well-structured logarithm with custom base and domain validation.The
logfunction properly handles:
- Domain errors for negative values and zero (via m_log)
- Optional base parameter with division-by-zero check (line 117)
- Consistent EDOM errors for both numerator and denominator
One edge case to verify:
log(x, 1)correctly returns EDOM at line 118 sinceden == 0.0.
169-172: Thecbrtimplementation is correct. Thecan_overflow=falseparameter is appropriate because the cube root function mathematically cannot produce infinite results for finite inputs—the result will always be finite. This is consistent with the C standard library behavior and Python's math module. No test failures are evident in the codebase.src/cmath/misc.rs (1)
152-206: Similar exception handling needed in polar test.The
test_polar_implfunction already handles Python exceptions (lines 197-203), which is the correct approach. Apply the same pattern totest_phase_implandtest_rect_impl.src/m.rs (1)
219-240: Well-implemented macOS-specific sincos optimization.The platform-specific
sincosoptimization using__sincos_streton macOS (lines 228-240) with a clean fallback for other platforms (lines 244-247) is correctly implemented. TheSinCosResultstruct layout matches Apple's ABI expectations.src/cmath/trigonometric.rs (2)
94-137: Robust overflow prevention in cosh with magnitude-based branching.The
coshimplementation correctly handles overflow for large real components (line 122) by factoring out e and computingcosh(x-1)*e, preventing spurious overflow when the final result is representable. The special-case handling for infinities (lines 97-117) is comprehensive.
295-328: atanh implementation handles challenging near-pole regions.The
atanhfunction includes sophisticated handling for:
- Large arguments (lines 306-312): asymptotic approximation
- Near-pole case
z.re == 1.0(lines 313-322): special formula to avoid cancellation- General case (lines 324-327): numerically stable with
mul_addThis demonstrates careful attention to numerical stability across the complex plane.
src/m_sys.rs (3)
36-39: Correct platform-specific link name handling for hypot on MSVC.The use of
#[cfg_attr(target_env = "msvc", link_name = "_hypot")]andlink_name = "_hypotf"correctly handles MSVC's naming convention where these functions are prefixed with underscore.
1-97: Clean FFI layer with appropriate future-proofing.The extern block provides a comprehensive libm interface with:
- Proper use of raw pointers for out-parameters (frexp, modf, lgammaf_r)
- Platform-specific configurations where needed
- Commented sections for future f128 and f32 support
The
#[allow(dead_code)]is appropriate since wrapper layer inm.rsselectively uses these.
64-65: The AIX exclusion forlgammaf_ris correct and should remain.AIX's standard libm does not provide the single-precision reentrant variant
lgammaf_r. While AIX supportslgamma_r(double precision), the single-precision reentrant function is not available in the standard library. The MASS libraries provide alternatives likelgammaf, but notlgammaf_r. The#[cfg(not(target_os = "aix"))]exclusion is necessary and appropriate to prevent build failures on AIX systems.src/math/integer.rs (3)
14-31:gcd/lcmbehavior matches typical math semantics (incl. sign + zeros).
lcmcorrectly returns 0 if either input is 0, and usesabsto ensure a non-negative result for signed inputs.
698-770: Tests are a solid baseline for the new APIs.Nice coverage of sign/zero handling (gcd/lcm) and basic correctness for isqrt/factorial/comb/perm.
68-124: The code correctly usesBigUint >> u64shifts, which bothnum-bigint 0.4andmalachite-bigint 0.2support. SinceBigUint::bits()returnsu64in both backends, the shift expressions at lines 99 and 110 (n >> (2 * c - 62)andn >> shift) are alreadyBigUint >> u64operations that work reliably across both backends. No conversion or adjustment is needed.Likely an incorrect or invalid review comment.
| proptest::proptest! { | ||
| #[test] | ||
| fn proptest_sin(re: f64, im: f64) { | ||
| test_sin(re, im); | ||
| } | ||
|
|
||
| #[test] | ||
| fn proptest_cos(re: f64, im: f64) { | ||
| test_cos(re, im); | ||
| } | ||
|
|
||
| #[test] | ||
| fn proptest_tan(re: f64, im: f64) { | ||
| test_tan(re, im); | ||
| } | ||
|
|
||
| #[test] | ||
| fn proptest_sinh(re: f64, im: f64) { | ||
| test_sinh(re, im); | ||
| } | ||
|
|
||
| #[test] | ||
| fn proptest_cosh(re: f64, im: f64) { | ||
| test_cosh(re, im); | ||
| } | ||
|
|
||
| #[test] | ||
| fn proptest_tanh(re: f64, im: f64) { | ||
| test_tanh(re, im); | ||
| } | ||
|
|
||
| #[test] | ||
| fn proptest_asin(re: f64, im: f64) { | ||
| test_asin(re, im); | ||
| } | ||
|
|
||
| #[test] | ||
| fn proptest_acos(re: f64, im: f64) { | ||
| test_acos(re, im); | ||
| } | ||
|
|
||
| #[test] | ||
| fn proptest_atan(re: f64, im: f64) { | ||
| test_atan(re, im); | ||
| } | ||
|
|
||
| #[test] | ||
| fn proptest_asinh(re: f64, im: f64) { | ||
| test_asinh(re, im); | ||
| } | ||
|
|
||
| #[test] | ||
| fn proptest_acosh(re: f64, im: f64) { | ||
| test_acosh(re, im); | ||
| } | ||
|
|
||
| #[test] | ||
| fn proptest_atanh(re: f64, im: f64) { | ||
| test_atanh(re, im); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the actual code in the specified file and lines
head -590 src/cmath/trigonometric.rs | tail -61Repository: RustPython/pymath
Length of output: 1327
🏁 Script executed:
# Search for test infrastructure and how tolerance is handled
rg "test_sin|test_cos|test_tan" --type rust -A 5 -B 2Repository: RustPython/pymath
Length of output: 11203
🏁 Script executed:
# Look for ULP tolerance or approximate comparison functions
rg "ulp|tolerance|approx" --type rust -iRepository: RustPython/pymath
Length of output: 1924
🏁 Script executed:
# Check if there's a test module with setup for proptest
rg "proptest" --type rust -B 5 -A 10Repository: RustPython/pymath
Length of output: 19135
🏁 Script executed:
# Find the test_cmath_func implementation
rg "fn test_cmath_func" -A 30 src/cmath.rsRepository: RustPython/pymath
Length of output: 1329
🏁 Script executed:
# Look at the complete ULP difference handling logic
rg "ulp_diff" -B 5 -A 10 src/cmath.rsRepository: RustPython/pymath
Length of output: 824
🏁 Script executed:
# Check if there are any failure conditions or assertions related to ULP
rg "panic|assert|error" src/cmath.rs | grep -i ulpRepository: RustPython/pymath
Length of output: 43
Fix test assertions to allow small ULP differences rather than requiring exact bit-matching.
The current implementation in assert_complex_eq (src/cmath.rs) contradicts its own comment: while the code states "allow small ULP difference," it panics when ulp_diff != 0. This enforces strict bit-exact matching, which is unrealistic for transcendental complex functions. A 1-2 ULP tolerance is appropriate since different implementations use different algorithms and approximation strategies.
Modify the ULP check in assert_complex_eq to accept a reasonable tolerance (e.g., if ulp_diff > 2) instead of panicking on any difference.
🧰 Tools
🪛 GitHub Actions: Rust
[error] 530-530: proptest_asinh failed: minimal failing input re=0.0396710..., im=9.85e-191 with ULP diff=1.
[error] 530-530: proptest_atan failed: minimal failing input re=3.14159..., im=6.28318... with ULP diff=1.
[error] 530-530: proptest_atanh failed: minimal failing input re=1e-10..., im=6.28318... with ULP diff=1.
[error] 530-530: proptest_tan failed: minimal failing input re≈-2.738e116, im≈-2.504e29 with ULP diff=1.
[error] 530-530: proptest_tanh failed: minimal failing input re≈-3.331e-206, im≈-2.504e29 with ULP diff=1.
🤖 Prompt for AI Agents
In @src/cmath/trigonometric.rs around lines 530 - 590, The assertion routine
assert_complex_eq (in src/cmath.rs) currently panics whenever ulp_diff != 0
which enforces bit-exact equality; change it to allow a small ULP tolerance
(e.g., accept differences up to 2 ULPs) by replacing the strict check with
something like if ulp_diff > 2 then panic, and update the failure message to
include the actual ulp_diff and the tolerated threshold so test output is
informative; keep the rest of the comparison logic intact so proptest-based trig
tests (proptest_* in trigonometric.rs) can tolerate tiny rounding differences.
| //! math.integer | ||
| //! | ||
| //! Integer-related mathematical functions. | ||
| //! This module requires either `num-bigint` or `malachite-bigint` feature. | ||
| #[cfg(feature = "malachite-bigint")] | ||
| use malachite_bigint::{BigInt, BigUint}; | ||
| #[cfg(feature = "num-bigint")] | ||
| use num_bigint::{BigInt, BigUint}; | ||
|
|
||
| use num_integer::Integer; | ||
| use num_traits::{One, Signed, ToPrimitive, Zero}; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "Cargo.toml" | head -20Repository: RustPython/pymath
Length of output: 73
🏁 Script executed:
git ls-files | grep -E "(Cargo\.toml|src/math)" | head -20Repository: RustPython/pymath
Length of output: 210
🏁 Script executed:
cat -n Cargo.tomlRepository: RustPython/pymath
Length of output: 1659
🏁 Script executed:
cat -n src/math/integer.rsRepository: RustPython/pymath
Length of output: 27557
🏁 Script executed:
cat -n src/math.rsRepository: RustPython/pymath
Length of output: 8207
🌐 Web query:
malachite-bigint 0.2 BigInt gcd Integer trait num-integer
💡 Result:
Short answer: malachite-bigint aims to be a drop‑in replacement for num-bigint, but gcd support lives in Malachite’s own traits/APIs rather than relying on num-integer’s Integer methods. Use malachite_base’s Gcd/GcdAssign traits or malachite-nz’s Integer/extended_gcd for GCD/extended-GCD functionality. See docs: malachite homepage and malachite_base Gcd trait, and malachite-nz’s Integer extended_gcd. [1][2][3]
Sources:
- Malachite project homepage and description. [1]
- malachite_base::num::arithmetic::gcd (Gcd / GcdAssign traits). [2]
- malachite-nz Integer extended_gcd docs. [3]
🏁 Script executed:
# Test if Cargo can compile with both features enabled
cd /tmp && cat > test_manifest.toml << 'EOF'
[package]
name = "test"
version = "0.0.1"
edition = "2021"
[dependencies]
num-bigint = "0.4"
num-integer = "0.1"
num-traits = "0.2"
malachite-bigint = "0.2"
EOF
# Try to compile a module with both imports
mkdir -p test_src
cat > test_src/main.rs << 'EOF'
#[cfg(feature = "malachite")]
use malachite_bigint::{BigInt, BigUint};
#[cfg(feature = "num")]
use num_bigint::{BigInt, BigUint};
fn main() {
println!("test");
}
EOF
# Check what happens with both cfg enabled
# Note: We can't actually compile without proper setup, but we can inspect the code structure
echo "Code structure check complete"Repository: RustPython/pymath
Length of output: 90
🏁 Script executed:
# Search for malachite-bigint documentation and API information
# Check what traits and methods it implements
rg "malachite.*bigint|malachite_bigint" --type rust -A 3 -B 1 2>/dev/null | head -50Repository: RustPython/pymath
Length of output: 486
🏁 Script executed:
# Check if there's actual usage/import of Integer trait that would require gcd
rg "Integer|gcd|Signed|ToPrimitive|Zero|One" src/math/integer.rs -nRepository: RustPython/pymath
Length of output: 872
🏁 Script executed:
# Look for any existing tests or CI that might test both features
git ls-files | grep -E "(\.yml|\.yaml|Makefile|tox\.ini|test)"Repository: RustPython/pymath
Length of output: 202
🌐 Web query:
malachite-bigint num_integer::Integer trait compatibility gcd implementation
💡 Result:
Short answer: malachite-bigint is intended as a drop‑in replacement for num-bigint and exposes malachite’s Integer/Natural functionality (including GCD/extended GCD). Malachite’s integer types implement Gcd/ExtendedGcd (with Bézout coefficients) and use Lehmer-style/GMP-like algorithms for fast GCD, so you get both compatibility and a fast gcd implementation. [1][2][3]
Relevant sources:
- malachite-bigint crate (drop‑in replacement for num-bigint). [1]
- malachite_base / Gcd / ExtendedGcd trait docs (GCD/extended GCD provided). [2]
- malachite performance notes: Lehmer’s/GMP-like GCD algorithms used (faster than simple binary/quadratic methods). [3]
If you want, I can:
- show the exact trait impls in malachite-bigint’s source (link and code lines), or
- give an example using malachite-bigint to compute gcd/extended_gcd.
🏁 Script executed:
cat -n .github/workflows/rust.ymlRepository: RustPython/pymath
Length of output: 1676
🏁 Script executed:
# Check if there are any existing Cargo.lock or test configurations that exercise both features
git ls-files | xargs grep -l "malachite-bigint\|num-bigint" 2>/dev/nullRepository: RustPython/pymath
Length of output: 118
🏁 Script executed:
# Check malachite-bigint's Cargo.toml or docs to see if it implements num_integer::Integer
python3 << 'EOF'
import json
import urllib.request
# Try to fetch malachite-bigint crate metadata from crates.io
try:
with urllib.request.urlopen('https://crates.io/api/v1/crates/malachite-bigint/0.2.0') as response:
data = json.loads(response.read().decode())
deps = data.get('crate', {}).get('dependencies', [])
print("malachite-bigint 0.2.0 dependencies:")
for dep in deps[:15]: # First 15
print(f" - {dep.get('crate_id')}: {dep.get('req')}")
except Exception as e:
print(f"Could not fetch from crates.io: {e}")
EOFRepository: RustPython/pymath
Length of output: 220
🏁 Script executed:
# Try to look up specific trait implementations or check if there are any docs
curl -s "https://docs.rs/malachite-bigint/0.2.0/" 2>/dev/null | grep -i "integer\|gcd" | head -10 || echo "Could not fetch docs.rs"Repository: RustPython/pymath
Length of output: 43
🏁 Script executed:
# Check what the actual compiler error would be by looking at the import structure
# Create a simple test case
cat > /tmp/test_imports.rs << 'EOF'
// Simulating what happens with both features enabled
#[cfg(feature = "malachite")]
use malachite_bigint::{BigInt, BigUint};
#[cfg(feature = "num")]
use num_bigint::{BigInt, BigUint};
// If both are true, we get:
// use malachite_bigint::{BigInt, BigUint};
// use num_bigint::{BigInt, BigUint};
// Both try to import BigInt and BigUint into the same namespace -> ERROR
EOF
echo "=== Simulated code structure analysis ==="
echo "When both features enabled, both use statements execute:"
cat /tmp/test_imports.rs
echo ""
echo "Result: Rust compiler error - name collision (BigInt and BigUint imported twice)"Repository: RustPython/pymath
Length of output: 611
Guard against "both bigint features enabled" (causes import name collision at compile time).
Enabling both malachite-bigint and num-bigint in Cargo.toml will cause both #[cfg] blocks to evaluate to true, resulting in conflicting BigInt/BigUint imports in the same scope. The parent module src/math.rs does protect against neither feature being enabled (the integer module is cfg-gated with #[cfg(feature = "_bigint")]), but there is no guard preventing both features from being enabled simultaneously.
Additionally, the current CI workflow (.github/workflows/rust.yml lines 46–48) tests each feature separately but never tests both features together, so the import collision bug is not caught.
Proposed fix
//! math.integer
//!
//! Integer-related mathematical functions.
//! This module requires either `num-bigint` or `malachite-bigint` feature.
+#[cfg(all(feature = "malachite-bigint", feature = "num-bigint"))]
+compile_error!("Enable only one of: `malachite-bigint` or `num-bigint`.");
+
#[cfg(feature = "malachite-bigint")]
use malachite_bigint::{BigInt, BigUint};
#[cfg(feature = "num-bigint")]
use num_bigint::{BigInt, BigUint};Note: The code uses a.gcd(b) which requires the num_integer::Integer trait. If malachite-bigint implements this trait for compatibility as advertised, the API usage is correct; however, this compatibility is not explicitly verified in the test suite.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| //! math.integer | |
| //! | |
| //! Integer-related mathematical functions. | |
| //! This module requires either `num-bigint` or `malachite-bigint` feature. | |
| #[cfg(feature = "malachite-bigint")] | |
| use malachite_bigint::{BigInt, BigUint}; | |
| #[cfg(feature = "num-bigint")] | |
| use num_bigint::{BigInt, BigUint}; | |
| use num_integer::Integer; | |
| use num_traits::{One, Signed, ToPrimitive, Zero}; | |
| //! math.integer | |
| //! | |
| //! Integer-related mathematical functions. | |
| //! This module requires either `num-bigint` or `malachite-bigint` feature. | |
| #[cfg(all(feature = "malachite-bigint", feature = "num-bigint"))] | |
| compile_error!("Enable only one of: `malachite-bigint` or `num-bigint`."); | |
| #[cfg(feature = "malachite-bigint")] | |
| use malachite_bigint::{BigInt, BigUint}; | |
| #[cfg(feature = "num-bigint")] | |
| use num_bigint::{BigInt, BigUint}; | |
| use num_integer::Integer; | |
| use num_traits::{One, Signed, ToPrimitive, Zero}; | |
🤖 Prompt for AI Agents
In @src/math/integer.rs around lines 1 - 13, Add a compile-time guard that
errors if both bigint features are enabled to avoid import collisions: insert a
cfg check like #[cfg(all(feature = "malachite-bigint", feature = "num-bigint"))]
followed by a compile_error! with a short message instructing users to enable
only one bigint feature; place this near the top of the integer module where
BigInt/BigUint are imported so the conflict is caught early. Also update CI to
include a matrix job that enables both features simultaneously to ensure the
guard triggers in CI. Ensure references to BigInt and BigUint (the imports
guarded by cfg(feature = "...")) remain unchanged aside from the added
compile-time check.
| /// Calculate C(n, k) or P(n, k) for n in the 63-bit range. | ||
| fn perm_comb_small(n: u64, k: u64, is_comb: bool) -> BigUint { | ||
| if k == 0 { | ||
| return BigUint::one(); | ||
| } | ||
|
|
||
| if is_comb { | ||
| // Fast path 1: use lookup tables for small n | ||
| if (k as usize) < FAST_COMB_LIMITS1.len() && n <= FAST_COMB_LIMITS1[k as usize] as u64 { | ||
| let comb_odd_part = REDUCED_FACTORIAL_ODD_PART[n as usize] | ||
| .wrapping_mul(INVERTED_FACTORIAL_ODD_PART[k as usize]) | ||
| .wrapping_mul(INVERTED_FACTORIAL_ODD_PART[(n - k) as usize]); | ||
| let shift = FACTORIAL_TRAILING_ZEROS[n as usize] as i32 | ||
| - FACTORIAL_TRAILING_ZEROS[k as usize] as i32 | ||
| - FACTORIAL_TRAILING_ZEROS[(n - k) as usize] as i32; | ||
| return BigUint::from(comb_odd_part << shift); | ||
| } | ||
|
|
||
| // Fast path 2: sequential multiplication for medium values | ||
| if (k as usize) < FAST_COMB_LIMITS2.len() && n <= FAST_COMB_LIMITS2[k as usize] { | ||
| let mut result = n; | ||
| let mut n = n; | ||
| let mut i = 1u64; | ||
| while i < k { | ||
| n -= 1; | ||
| result *= n; | ||
| i += 1; | ||
| result /= i; | ||
| } | ||
| return BigUint::from(result); | ||
| } | ||
| } else { | ||
| // Permutation fast paths | ||
| if (k as usize) < FAST_PERM_LIMITS.len() && n <= FAST_PERM_LIMITS[k as usize] { | ||
| if n <= 127 { | ||
| let perm_odd_part = REDUCED_FACTORIAL_ODD_PART[n as usize] | ||
| .wrapping_mul(INVERTED_FACTORIAL_ODD_PART[(n - k) as usize]); | ||
| let shift = FACTORIAL_TRAILING_ZEROS[n as usize] as i32 | ||
| - FACTORIAL_TRAILING_ZEROS[(n - k) as usize] as i32; | ||
| return BigUint::from(perm_odd_part << shift); | ||
| } | ||
|
|
||
| let mut result = n; | ||
| let mut n = n; | ||
| let mut i = 1u64; | ||
| while i < k { | ||
| n -= 1; | ||
| result *= n; | ||
| i += 1; | ||
| } | ||
| return BigUint::from(result); | ||
| } | ||
| } | ||
|
|
||
| // For larger n use recursive formulas: | ||
| // P(n, k) = P(n, j) * P(n-j, k-j) | ||
| // C(n, k) = C(n, j) * C(n-j, k-j) / C(k, j) | ||
| let j = k / 2; | ||
| let a = perm_comb_small(n, j, is_comb); | ||
| let b = perm_comb_small(n - j, k - j, is_comb); | ||
| let mut result = a * b; | ||
| if is_comb { | ||
| let c = perm_comb_small(k, j, true); | ||
| result /= c; | ||
| } | ||
| result | ||
| } |
There was a problem hiding this comment.
Fix shifting by i32 + avoid shifting the u64 before converting to BigUint.
comb_odd_part << shift / perm_odd_part << shift uses shift: i32; shifting an integer by an i32 variable is very likely to fail to compile. Also, shifting after converting to BigUint avoids any accidental u64 overflow concerns.
Proposed fix
- let shift = FACTORIAL_TRAILING_ZEROS[n as usize] as i32
+ let shift = FACTORIAL_TRAILING_ZEROS[n as usize] as i32
- FACTORIAL_TRAILING_ZEROS[k as usize] as i32
- FACTORIAL_TRAILING_ZEROS[(n - k) as usize] as i32;
- return BigUint::from(comb_odd_part << shift);
+ debug_assert!(shift >= 0);
+ return BigUint::from(comb_odd_part) << (shift as usize);
...
- let shift = FACTORIAL_TRAILING_ZEROS[n as usize] as i32
+ let shift = FACTORIAL_TRAILING_ZEROS[n as usize] as i32
- FACTORIAL_TRAILING_ZEROS[(n - k) as usize] as i32;
- return BigUint::from(perm_odd_part << shift);
+ debug_assert!(shift >= 0);
+ return BigUint::from(perm_odd_part) << (shift as usize);
youknowone
force-pushed
the
math
branch
4 times, most recently
from
b22306e to
c468a2a
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @src/cmath/trigonometric.rs:
- Around line 313-322: Add a clarifying comment above the branch handling z.re
== 1.0 && ay < CM_SQRT_DBL_MIN that explains this is the C99/CPython
special-case for atanh near the real argument 1 (to produce +inf +/- 0i for
ay==0 and a stable finite result for tiny positive ay), and document the math:
show that r_re = -log(sqrt(ay)/sqrt(hypot(ay,2))) is the stable form of
0.5*log((1+|y|)/(1-|y|)) for tiny |y| and r_im = copysign(atan2(2.0, -ay)/2.0,
z.im) yields the correct sign and quadrant for the imaginary part; include a
short note pointing to CPython’s implementation or the C99 reference as the
source for this formula (mention CPython and C99 by name).
- Around line 254-266: The conditional creating r_re in the large-argument
branch is redundant because both branches compute the same
copysign(log(hypot(...)) + M_LN2*2.0, z.re); replace the if z.im >= 0.0 { ... }
else { ... } block with a single assignment: set r_re using
m::copysign(m::log(m::hypot(z.re / 2.0, z.im / 2.0)) + M_LN2 * 2.0, z.re) and
remove the entire conditional; keep r_im and the return as-is.
In @src/err.rs:
- Around line 63-68: The Windows branch in get_errno uses the invalid `safe fn
_errno()` declaration; remove the `safe` keyword and declare the external
function as `extern "C" { fn _errno() -> *mut i32; }` and perform the pointer
access inside an unsafe block (mirroring the fix applied to set_errno) so the
code compiles on Windows and safely reads the errno value.
🧹 Nitpick comments (5)
src/math/integer.rs (1)
561-626: Consider potential performance impact of recursive calls for large k.The
perm_comb_smallfunction uses recursion (j = k / 2) which could lead to deep call stacks for very largekvalues. While this is unlikely to be a problem in practice (recursion depth is O(log k)), consider whether an iterative approach might be beneficial for extreme cases.src/math/exponential.rs (1)
125-140: Consider unifyingmath_1_fnwithsuper::math_1.This helper duplicates most of the logic from
super::math_1(Lines 52-72 in src/math.rs). The only difference is thatmath_1_fndoesn't callset_errno(0)or check errno afterward. If this is intentional (because the internalm_*functions don't touch errno), consider adding a comment to clarify the design decision.src/m_sys.rs (1)
66-66: Verify the signature forlgammaf_r- using&mut i32in extern "C" is unusual.The declaration
pub fn lgammaf_r(n: f32, s: &mut i32) -> f32uses a Rust reference in an extern "C" signature. While this works because&mut i32has the same ABI as*mut i32, the idiomatic pattern for FFI is to use raw pointers (likefrexpandmodfdo on lines 52-54). Consider changing to*mut i32for consistency.♻️ Suggested fix for consistency
#[cfg(not(target_os = "aix"))] - pub fn lgammaf_r(n: f32, s: &mut i32) -> f32; + pub fn lgammaf_r(n: f32, s: *mut i32) -> f32;src/cmath/trigonometric.rs (2)
214-220: Consider extracting intermediate variables for clarity.The expression on line 219 is dense and difficult to verify:
Complex64::new(tx * mul_add(ty, ty, 1.0) / denom, ((ty / denom) * cx) * cx)♻️ Optional refactoring for improved readability
let tx = m::tanh(z.re); let ty = m::tan(z.im); let cx = 1.0 / m::cosh(z.re); let txty = tx * ty; let denom = mul_add(txty, txty, 1.0); -let r = Complex64::new(tx * mul_add(ty, ty, 1.0) / denom, ((ty / denom) * cx) * cx); +let ty_sq_plus_1 = mul_add(ty, ty, 1.0); +let r_re = tx * ty_sq_plus_1 / denom; +let r_im = (ty / denom) * cx * cx; +let r = Complex64::new(r_re, r_im);
324-327: Consider adding comments for the standard case formulas.The expressions on lines 325-326 are mathematically dense:
let r_re = m::log1p(4.0 * z.re / mul_add(one_minus_re, one_minus_re, ay * ay)) / 4.0; let r_im = -m::atan2(-2.0 * z.im, mul_add(one_minus_re, 1.0 + z.re, -(ay * ay))) / 2.0;While the implementation may be correct, adding a brief comment explaining the formula or referencing the source would significantly improve maintainability:
+// Standard formula: atanh(x + iy) = (1/4)*log(1 + 4x/((1-x)² + y²)) - (i/2)*atan2(-2y, (1-x)(1+x) - y²) let one_minus_re = 1.0 - z.re; let r_re = m::log1p(4.0 * z.re / mul_add(one_minus_re, one_minus_re, ay * ay)) / 4.0;
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
.github/workflows/rust.yml.python-versionAGENTS.mdCargo.tomlREADME.mdproptest-regressions/cmath.txtproptest-regressions/cmath/misc.txtproptest-regressions/cmath/trigonometric.txtproptest-regressions/gamma.txtproptest-regressions/math.txtproptest-regressions/math/aggregate.txtproptest-regressions/math/exponential.txtproptest-regressions/math/gamma.txtsrc/cmath.rssrc/cmath/exponential.rssrc/cmath/misc.rssrc/cmath/trigonometric.rssrc/err.rssrc/lib.rssrc/m.rssrc/m_sys.rssrc/math.rssrc/math/aggregate.rssrc/math/exponential.rssrc/math/gamma.rssrc/math/integer.rssrc/math/misc.rssrc/math/trigonometric.rssrc/test.rs
💤 Files with no reviewable changes (1)
- proptest-regressions/gamma.txt
✅ Files skipped from review due to trivial changes (2)
- proptest-regressions/cmath/trigonometric.txt
- AGENTS.md
🚧 Files skipped from review as they are similar to previous changes (8)
- .python-version
- proptest-regressions/cmath.txt
- src/math/aggregate.rs
- src/math/misc.rs
- src/lib.rs
- src/cmath.rs
- Cargo.toml
- README.md
🧰 Additional context used
🧬 Code graph analysis (9)
src/cmath/misc.rs (3)
src/cmath.rs (6)
c(41-43)special_type(70-90)result(144-144)e(164-164)e(169-169)assert_complex_eq(97-126)src/err.rs (2)
set_errno(26-49)get_errno(53-76)src/cmath/trigonometric.rs (2)
cos(226-229)sin(234-238)
src/math/trigonometric.rs (4)
src/m.rs (13)
sin(33-35)cos(28-30)tan(38-40)asin(13-15)acos(8-10)atan(18-20)atan2(23-25)sinh(65-67)cosh(60-62)tanh(70-72)asinh(50-52)acosh(45-47)atanh(55-57)src/m_sys.rs (13)
sin(12-12)cos(11-11)tan(13-13)asin(8-8)acos(7-7)atan(9-9)atan2(10-10)sinh(20-20)cosh(19-19)tanh(21-21)asinh(17-17)acosh(16-16)atanh(18-18)src/math.rs (2)
math_1(53-72)math_2(78-87)src/test.rs (2)
test_math_1(76-95)test_math_2(98-147)
src/math/exponential.rs (5)
src/m.rs (12)
exp(77-79)exp2(82-84)expm1(87-89)log(92-94)log10(97-99)log2(107-109)log1p(102-104)sqrt(129-131)cbrt(114-116)pow(124-126)fmod(161-163)copysign(141-143)src/m_sys.rs (12)
exp(24-24)exp2(25-25)expm1(26-26)log(28-28)log10(29-29)log2(32-32)log1p(30-30)sqrt(42-42)cbrt(35-35)pow(41-41)fmod(51-51)copysign(46-46)src/math.rs (1)
math_1(53-72)src/math/misc.rs (2)
fmod(118-128)copysign(16-18)src/test.rs (2)
test_math_1(76-95)test_math_2(98-147)
src/test.rs (2)
src/math/aggregate.rs (2)
e(404-404)e(412-412)src/math/misc.rs (1)
e(400-400)
src/math/gamma.rs (4)
src/lib.rs (1)
mul_add(20-26)src/m.rs (2)
erf(198-200)erfc(203-205)src/m_sys.rs (2)
erf(60-60)erfc(61-61)src/math.rs (1)
math_1a(93-97)
src/m.rs (5)
src/m_sys.rs (37)
acos(7-7)asin(8-8)atan(9-9)atan2(10-10)cos(11-11)sin(12-12)tan(13-13)acosh(16-16)asinh(17-17)atanh(18-18)cosh(19-19)sinh(20-20)tanh(21-21)exp(24-24)exp2(25-25)expm1(26-26)log(28-28)log10(29-29)log1p(30-30)log2(32-32)cbrt(35-35)hypot(38-38)pow(41-41)sqrt(42-42)ceil(45-45)copysign(46-46)fabs(47-47)floor(50-50)fmod(51-51)frexp(52-52)ldexp(53-53)modf(54-54)nextafter(55-55)remainder(56-56)trunc(57-57)erf(60-60)erfc(61-61)src/math/trigonometric.rs (13)
acos(35-37)asin(28-30)atan(42-44)atan2(49-51)cos(15-17)sin(9-11)tan(21-23)acosh(81-83)asinh(75-77)atanh(87-89)cosh(63-65)sinh(57-59)tanh(69-71)src/math/exponential.rs (10)
exp(9-11)exp2(15-17)expm1(21-23)log(95-123)log10(144-146)log1p(156-158)log2(150-152)cbrt(170-172)pow(176-236)sqrt(164-166)src/math.rs (1)
hypot(104-110)src/math/gamma.rs (2)
erf(8-10)erfc(14-16)
src/cmath/trigonometric.rs (3)
src/cmath.rs (3)
c(41-43)special_type(70-90)test_cmath_func(128-180)src/cmath/exponential.rs (4)
sqrt(57-86)exp(90-132)log(136-168)test_cmath_func(181-186)src/lib.rs (1)
mul_add(20-26)
src/math.rs (5)
src/m.rs (17)
cbrt(114-116)exp(77-79)exp2(82-84)expm1(87-89)log(92-94)log1p(102-104)log2(107-109)log10(97-99)pow(124-126)sqrt(129-131)acos(8-10)atan(18-20)atan2(23-25)cos(28-30)sin(33-35)tan(38-40)hypot(119-121)src/math/exponential.rs (10)
cbrt(170-172)exp(9-11)exp2(15-17)expm1(21-23)log(95-123)log1p(156-158)log2(150-152)log10(144-146)pow(176-236)sqrt(164-166)src/math/trigonometric.rs (6)
acos(35-37)atan(42-44)atan2(49-51)cos(15-17)sin(9-11)tan(21-23)src/err.rs (3)
set_errno(26-49)get_errno(53-76)is_error(80-97)src/cmath/misc.rs (1)
abs(96-98)
src/m_sys.rs (4)
src/m.rs (37)
acos(8-10)asin(13-15)atan(18-20)atan2(23-25)cos(28-30)sin(33-35)tan(38-40)acosh(45-47)asinh(50-52)atanh(55-57)cosh(60-62)sinh(65-67)tanh(70-72)exp(77-79)exp2(82-84)expm1(87-89)log(92-94)log10(97-99)log1p(102-104)log2(107-109)cbrt(114-116)hypot(119-121)pow(124-126)sqrt(129-131)ceil(136-138)copysign(141-143)fabs(146-148)floor(156-158)fmod(161-163)frexp(166-168)ldexp(171-173)modf(176-178)nextafter(181-183)remainder(186-188)trunc(191-193)erf(198-200)erfc(203-205)src/math/trigonometric.rs (13)
acos(35-37)asin(28-30)atan(42-44)atan2(49-51)cos(15-17)sin(9-11)tan(21-23)acosh(81-83)asinh(75-77)atanh(87-89)cosh(63-65)sinh(57-59)tanh(69-71)src/math/exponential.rs (10)
exp(9-11)exp2(15-17)expm1(21-23)log(95-123)log10(144-146)log1p(156-158)log2(150-152)cbrt(170-172)pow(176-236)sqrt(164-166)src/math/gamma.rs (2)
erf(8-10)erfc(14-16)
🔇 Additional comments (63)
proptest-regressions/math/gamma.txt (1)
7-7: No action needed. The proptest regression file for gamma is correctly set up with a valid shrink target that the implementation properly handles.proptest-regressions/math/aggregate.txt (1)
1-7: LGTM! Proptest regression seed file follows standard conventions.This auto-generated proptest seed file follows the expected format and will help ensure previously discovered edge cases are re-tested before generating new test cases.
proptest-regressions/cmath/misc.txt (1)
1-7: LGTM! Proptest regression seed file is properly formatted.This auto-generated seed file for complex math tests follows proptest conventions and will help maintain test coverage of previously discovered edge cases.
.github/workflows/rust.yml (2)
14-47: Good feature matrix coverage across platforms.The updated CI workflow effectively tests different OS and feature combinations:
- Cross-platform testing (Ubuntu, Windows, macOS)
- Multiple big-integer implementations (num-bigint, malachite-bigint)
- Both debug and release builds with num-bigint
The
mul_addfeature being enabled only on macOS appears intentional, likely due to platform-specific FMA support.
33-33: Python 3.14 was released on October 7, 2025, and is available throughout 2026 with regular bugfix releases. GitHub Actions supports Python 3.14 viaactions/setup-python@v6withpython-version: '3.14'. The workflow configuration is correct and supported.proptest-regressions/math/exponential.txt (1)
1-10: LGTM! Multiple regression seeds properly captured.This proptest seed file correctly captures four regression cases for exponential functions. The format is standard and will ensure these edge cases are consistently re-tested.
proptest-regressions/math.txt (1)
1-7: LGTM! Proptest regression seed file is properly formatted.This seed file correctly captures a regression case with very small floating-point values, which is important for testing edge cases near the subnormal range.
src/math/gamma.rs (4)
1-16: LGTM! Clean error function implementations.The
erfanderfcfunctions follow the established pattern of delegating tomath_1awith FFI function pointers fromcrate::m. The errno-based error handling is properly integrated.
70-71: Good refactoring to use centralizedmul_add.Moving from a local implementation to
crate::mul_addeliminates duplication and ensures consistent behavior across the crate.
21-25: Appropriate use of clippy attributes for mathematical constants.The
#[allow(clippy::excessive_precision)]attributes are correctly applied to Lanczos coefficients that require full precision for numerical accuracy.
271-279: Excellent test coverage for the new error functions.The property-based tests for
erfanderfcfollow the established testing pattern and ensure cross-validation with Python's implementation.src/err.rs (1)
78-97: Well-designed errno-to-Result mapping.The
is_errorfunction properly handles libm error conditions:
- The 1.5 threshold for
ERANGEcorrectly handles subnormal underflow cases- The reference to bpo-46018 provides good context
- Treating unknown errno values as success is a reasonable defensive choice
src/cmath/exponential.rs (4)
52-86: Robust complex square root implementation.The function correctly handles:
- Special values via the macro
- Subnormal numbers with appropriate scaling (CM_SCALE_UP/DOWN)
- Normal numbers with the /8.0 optimization to avoid overflow
- Sign preservation using
copysign
88-132: Well-implemented complex exponential with overflow protection.The
expfunction demonstrates careful handling of:
- Non-finite inputs with appropriate special values
- Overflow prevention using
CM_LOG_LARGE_DOUBLEthreshold (line 117)- Proper EDOM error for invalid infinite imaginary cases
- Efficient computation using
sincos
134-168: Numerically stable complex logarithm implementation.The multi-path approach ensures accuracy across all magnitudes:
- Large values: scaled to prevent overflow
- Subnormals: scaled to prevent underflow
- Values near 1: use
log1pto avoid cancellation (lines 157-160)The range
(0.71..=1.73)for the log1p path is well-chosen for numerical stability.
170-260: Simple log10 implementation with excellent test coverage.The
log10function correctly delegates tologand scales by1/M_LN10. The comprehensive test suite with edge cases and property-based testing ensures correctness across all input ranges.src/cmath/misc.rs (5)
18-29: Phase function correctly handles errno from atan2.The errno handling is appropriate:
- Sets errno to 0 before the call
- Maps
EDOMandERANGEcorrectly- Line 27: Unknown errno values are treated as
EDOM(matching Python'sPyErr_SetFromErrno)
31-40: Correct polar coordinate conversion with overflow detection.The overflow check on line 36 correctly detects when
hypotoverflows to infinity even though both inputs are finite, returningERANGEas appropriate.
42-74: Comprehensive rect implementation with platform workaround.The function handles numerous edge cases correctly:
- Infinite radius with finite angle (lines 50-58)
- Domain errors for invalid combinations (lines 63-64)
- Platform-specific workaround for OS X 10.8 (lines 68-69)
The complexity is justified by the need to match IEEE 754 and Python semantics.
76-117: Well-implemented utility functions.All utility functions are correctly implemented:
isfinite,isnan,isinf: straightforward predicatesabs: useshypotfor correct magnitude calculationisclose: includes early exact-equality shortcut and proper infinity handling
119-369: Excellent comprehensive test coverage.The test suite is exemplary:
- Cross-validation with Python
cmathfor correctness- Edge case testing across all special values
- Property-based testing for random inputs
- Both positive and negative test cases
This ensures robust validation of the complex number utilities.
src/test.rs (3)
4-47: Comprehensive edge value coverage for testing.The
EDGE_VALUESarray provides excellent coverage of floating-point edge cases:
- IEEE 754 special values (zeros, infinities, NaNs with different payloads)
- Boundary values (MIN_POSITIVE, MAX, MIN)
- Subnormal values
- Various scales (1e-300 to 1e308)
- Trigonometric special points (π, π/2, etc.)
This ensures thorough testing across the floating-point domain.
75-95: Clean test helper for 1-argument functions.The
test_math_1function provides a solid pattern for cross-validation:
- Compares Rust implementation against Python's math module
- Handles NaN comparison correctly (lines 86-88)
- Uses bit-level comparison for exact matching
- Provides clear error messages with function name and input
97-147: Robust test helper for 2-argument functions.The
test_math_2function correctly handles:
- Successful calls with bit-level comparison
- NaN edge cases (lines 117-119)
- Error case validation, mapping Python exceptions to Rust errors:
ValueError→EDOM(lines 129-134)OverflowError→ERANGE(lines 135-140)src/math/integer.rs (6)
1-12: LGTM! Feature-gated imports are correctly structured.The conditional compilation for
malachite-bigintvsnum-bigintis properly set up, ensuring mutual exclusivity at the feature level.
14-31: LGTM! GCD and LCM implementations are correct.The
gcddelegates to the optimizednum-integerimplementation, andlcmcorrectly handles zero cases and ensures the result is always non-negative via.abs().
54-124: LGTM! Well-implemented adaptive-precision integer square root.The
isqrtfunction correctly implements Newton's iteration with:
- Fast path for small numbers (≤64-bit)
- Slow path using BigUint for large numbers
- Proper final correction to handle off-by-one
186-228: LGTM! Efficient factorial using divide-and-conquer.The implementation correctly uses:
- Lookup table for small values (0-20)
- Binary splitting algorithm for larger values
- Proper two-valuation calculation for the trailing zeros optimization
657-696: LGTM! Public API for comb and perm is well-designed.Both functions correctly:
- Handle edge cases (k > n returns 0)
- Optimize
combby using the smaller of k or n-k- Delegate to the optimized
perm_comb_smallhelper
698-770: LGTM! Test coverage looks comprehensive.Tests cover basic cases, edge cases (zeros, negatives for gcd/lcm), and verify key properties like symmetry for comb.
src/math/exponential.rs (5)
7-23: LGTM! Exponential function wrappers are correctly implemented.All three functions properly delegate to
super::math_1withcan_overflow: true, which is correct since exp functions can overflow to infinity.
27-91: LGTM! Log helper functions handle edge cases correctly.The
m_log,m_log10,m_log2, andm_log1pfunctions properly handle:
- Positive finite values → compute result
- Zero →
-inf- Negative values →
NaN+inf→+inf-inf→NaNNaN→NaN
93-123: LGTM! Thelogfunction with optional base is well-implemented.Correctly handles:
- Domain errors for invalid inputs
- Division by zero when
base == 1(returnsEDOM)- Proper error propagation for both numerator and denominator
174-236: LGTM! Comprehensive IEEE-754 compliantpowimplementation.The function correctly handles all special cases:
NaN**0 = 1,1**NaN = 1- Infinite base with positive/negative/zero exponents
- Infinite exponent with various base magnitudes
- Domain errors for
(-ve)**(non-integer)- Distinguishes overflow (
ERANGE) from divide-by-zero (EDOM) for0**negative
240-406: LGTM! Thorough test coverage with proptest and edge cases.The test suite validates all functions against Python's
mathmodule using both property-based testing and explicit edge value testing.src/math/trigonometric.rs (3)
7-51: LGTM! Trigonometric function wrappers are correctly implemented.All functions properly delegate to
super::math_1orsuper::math_2withcan_overflow: false, which is correct since standard trig functions are bounded (except at domain errors likeasin(2)).
53-89: LGTM! Hyperbolic function wrappers correctly handle overflow.
sinhandcoshcorrectly usecan_overflow: truesince they can overflow to infinity for large inputs.tanh,asinh,acosh, andatanhusecan_overflow: falsewhich is appropriate for their behavior.
91-298: LGTM! Comprehensive test coverage for all trigonometric and hyperbolic functions.Tests include both edge case testing using
EDGE_VALUESand property-based testing via proptest, validating against Python'smathmodule.src/math.rs (7)
1-22: LGTM! Clean module structure with comprehensive re-exports.The module organization mirrors Python's
mathmodule structure, with submodules for related functionality and a feature-gatedintegermodule for big-integer operations.
24-46: LGTM! Useful macro for generating simple libm wrappers.The
libm_simplemacro provides a clean way to generate 1-arg and 2-arg wrappers that don't need errno handling. Good use of declarative macros.
52-72: LGTM! Well-documented error handling wrapper.The
math_1function correctly:
- Resets errno before the call
- Detects domain errors (NaN result from non-NaN input)
- Distinguishes overflow from domain errors based on
can_overflow- Has platform-specific errno checking (disabled on Windows/macOS where it's unnecessary)
74-87: LGTM! Consistent 2-argument error handling.The
math_2function follows the same pattern asmath_1, correctly detecting domain and range errors for two-argument functions.
99-110: Good design choice usingvector_norm_2instead of libmhypot.The comment explains the rationale: consistent results across platforms and better overflow/underflow handling. This aligns with Python's approach for numerical stability.
112-141: LGTM! Constants and angle conversions match Python's math module.Constants use Rust's
std::f64::constswhich are IEEE-754 compliant. Thedegreesandradiansfunctions use the standard conversion formulas.
143-241: LGTM! Tests validate against Python's math module.The test suite uses PyO3 to compare results with Python, ensuring compatibility. Both edge cases and property-based tests are included.
src/m.rs (4)
7-72: LGTM! Safe wrappers for trigonometric and hyperbolic functions.All wrappers correctly delegate to
m_syswith appropriateunsafeblocks. The#[inline(always)]attribute ensures no overhead for these thin wrappers.
74-131: LGTM! Safe wrappers for exponential, logarithmic, and power functions.All wrappers follow the same consistent pattern, correctly encapsulating the unsafe FFI calls.
165-178: LGTM! Safe wrappers forfrexpandmodfcorrectly handle pointer parameters.These functions take
&mutreferences and pass them as raw pointers to the FFI, providing a safe Rust interface over the C-style output parameters.
217-247: LGTM! Platform-specificsincosoptimization is well-implemented.The macOS path uses Apple's optimized
__sincos_stretwhich computes both values in a single call. The non-macOS fallback correctly computes them separately. Both paths are gated behindfeature = "complex".src/m_sys.rs (4)
1-5: LGTM! FFI block is correctly configured.The
#[cfg_attr(unix, link(name = "m"))]ensures libm is linked on Unix systems. The#[allow(dead_code)]is appropriate since not all declared functions may be used in all configurations.
6-57: LGTM! Core libm function declarations are correct.All function signatures match the standard C libm interface. The MSVC-specific
link_nameattributes forhypot/hypotfcorrectly handle the underscore prefix convention on that platform.
59-68: LGTM! Special function declarations with appropriate platform guards.The
lgammaf_rexclusion for AIX (#[cfg(not(target_os = "aix"))]) is correct since AIX's libm doesn't provide this function.
70-98: LGTM! Future extension points are clearly documented.The commented-out f128 and additional f32 variants provide clear indication of planned future support. The MSVC x86 exclusion comment for some f32 functions notes a known platform limitation.
src/cmath/trigonometric.rs (10)
1-13: LGTM - Well-defined numerical constants.The imports and constants are correctly defined. The
CM_SQRT_LARGE_DOUBLEandCM_SQRT_DBL_MINthreshold values are used appropriately throughout the implementation for overflow/underflow protection.
14-91: LGTM - Comprehensive special value tables.The special value lookup tables systematically handle edge cases for infinities, NaNs, and zeros. The consistent 7×7 structure maps input special types to output values, with
U(undefined) markers for cases requiring computation.
93-137: LGTM - Robust cosh implementation with overflow protection.The function correctly handles special values, implements large-magnitude optimization to prevent spurious overflow (lines 122-126), and returns appropriate errors (EDOM for domain violations, ERANGE for overflow).
139-182: LGTM - Well-structured sinh implementation.The function follows the same robust pattern as
coshwith appropriate handling of special cases, large-magnitude optimization, and consistent error reporting.
223-247: LGTM - Clean trigonometric implementations via standard identities.The functions correctly implement complex trigonometric functions by delegating to their hyperbolic counterparts with appropriate rotations (
cos(z) = cosh(iz), etc.). This approach ensures consistency and leverages already-tested code.
275-292: LGTM - Clear acosh implementation.The function properly handles large arguments to avoid overflow and uses a numerically stable formulation with
sqrtandasinhfor the standard case.
356-370: LGTM - Elegant implementations via standard identities.Both
asinandatancorrectly implement the complex inverse trigonometric functions by delegating to their hyperbolic counterparts with appropriate rotations, following the identitiesasin(z) = -i * asinh(iz)andatan(z) = -i * atanh(iz).
372-591: LGTM - Comprehensive test coverage with edge cases and property-based testing.The test suite provides excellent coverage:
- Systematic edge-value testing for all 12 functions against predefined edge cases
- Property-based testing with
proptestfor random inputs- Consistent delegation to shared test infrastructure (
crate::cmath::tests::test_cmath_func)This testing approach should effectively validate numerical correctness and catch edge-case bugs.
335-347: The sign handling in the large argument case is correct and already comprehensively verified by the existing test infrastructure. Theedgetest_acos()function tests all 900 combinations of complex inputs (30×30 EDGE_VALUES pairs), andtest_cmath_funcvalidates each result against Python's cmath module, which implements the C99 standard. The conditional branches—negating and usingcopysignwithz.imfor negative real parts, andcopysignwith-z.imotherwise—correctly handle the different quadrants per C99 specification. No action required.
191-193: The use of2*sin(z)*cos(z)instead ofsin(2*z)is necessary for numerical correctness.The concern about overflow in
2.*z.im(line 205) is valid. For very large imaginary values approaching f64::MAX, computing2.0 * z.imoverflows to infinity, causingsin(infinity)to return NaN. By instead computing2*sin(z.im)*cos(z.im), the result remains bounded (both sin and cos are bounded to [-1, 1], so their product stays within [-2, 2]) and avoids the overflow issue. This is not premature optimization but necessary handling for edge cases with extreme imaginary parts.The same applies to line 209's use of
4.0 * m::sin(z.im) * m::cos(z.im)instead of2.0 * m::sin(2.0 * z.im).Likely an incorrect or invalid review comment.
| if m::fabs(z.re) > CM_LARGE_DOUBLE || m::fabs(z.im) > CM_LARGE_DOUBLE { | ||
| // Avoid overflow for large arguments | ||
| let r_re = if z.im >= 0.0 { | ||
| m::copysign(m::log(m::hypot(z.re / 2.0, z.im / 2.0)) + M_LN2 * 2.0, z.re) | ||
| } else { | ||
| -m::copysign( | ||
| m::log(m::hypot(z.re / 2.0, z.im / 2.0)) + M_LN2 * 2.0, | ||
| -z.re, | ||
| ) | ||
| }; | ||
| let r_im = m::atan2(z.im, m::fabs(z.re)); | ||
| return Ok(Complex64::new(r_re, r_im)); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/cmath/trigonometric.rs | sed -n '249,273p'Repository: RustPython/pymath
Length of output: 1173
🏁 Script executed:
# Find the asinh function definition
rg -n "fn asinh" src/cmath/ -A 30Repository: RustPython/pymath
Length of output: 2180
🌐 Web query:
CPython complex asinh implementation reference asinh large arguments
💡 Result:
Short answer: CPython's cmath.asinh calls the platform C complex-asinh (casinh/casinhf/casinhl) when available; where not available it falls back to the standard relations (e.g. asinh(z) = -i·asin(i·z) or the log formula asinh(z) = log(z + sqrt(z*z + 1)). For large |z| you therefore rely on the C library’s casinh (which implements numerically stable/overflow-avoiding algorithms) or on the standard log+sqrt fallback (implementations typically scale/choose formulas to avoid overflow). [1][2]
Sources:
- casinh / complex asinh (C/C++ reference, C99 behavior and branch cuts). [1]
- CPython source tree / repository (cmath functions are thin wrappers around the platform math library when available). [2]
References:
[1] cppreference: casinh / complex asinh.
[2] python/cpython repository (stdlib implementation wraps platform math library).
🏁 Script executed:
# Check other inverse hyperbolic functions for similar patterns
rg -n "if.*\\.im >= 0.0" src/cmath/trigonometric.rs -A 8 -B 2Repository: RustPython/pymath
Length of output: 147
🏁 Script executed:
# Look for git history or comments that might explain this pattern
git log -p --all -S "copysign.*-z.re" -- src/cmath/trigonometric.rs | head -100Repository: RustPython/pymath
Length of output: 147
🏁 Script executed:
# Check if there are tests that validate this behavior
fd -e rs -e toml . | xargs rg -l "asinh.*test|test.*asinh" | head -5Repository: RustPython/pymath
Length of output: 147
🏁 Script executed:
# Look for acosh function to compare pattern
sed -n '275,310p' src/cmath/trigonometric.rsRepository: RustPython/pymath
Length of output: 1411
🏁 Script executed:
# Search for atanh to see if it has the same pattern
rg -n "fn atanh" src/cmath/ -A 20Repository: RustPython/pymath
Length of output: 1588
🌐 Web query:
asinh complex large arguments copysign formula numerical stability
💡 Result:
Short answer and practical recipe:
-
Definition (principal branch):
asinh(z) = log(z + sqrt(z^2 + 1)). [DLMF]. [1] -
Numerical issue for large |z|:
When |z| is large, z and sqrt(z^2+1) are nearly equal and subtraction/cancellation can produce large relative error in the argument of log. Libraries therefore use algebraic rearrangements and careful branch/sign selection (copysign) to avoid cancellation and to pick the correct square‑root branch. [2][3] -
Stable evaluation recipe (practical pseudocode):
- Compute w = z*z + 1.
- r = csqrt(w). // principal complex sqrt
- /* choose sign of r so z + r avoids cancellation: pick r with sign matching z */
if (real(z) != 0) r = copysign(r, real(z));
else r = copysign(r, imag(z)); // tie-breaker when Re(z)==0 - return clog(z + r). // complex log of the sum
Rationale: forcing the sqrt to have the same sign (in the real part) as z makes z + r have magnitude ≈ 2|z| for large |z|, avoiding catastrophic cancellation and preserving the principal branch. This is the approach used in robust math libraries (fdlibm / glibc variants and other implementations) and is discussed in literature on branch cuts and stable complex elementary functions. [2][3][4]
-
Fast asymptotic alternative (for very large |z|, if you only need an approximation):
asinh(z) = log(2z) + O(1/z^2). Use log(2z) (with correct complex log branch) when |z| ≫ 1 to save work, but only if that accuracy suffices. [1][2]
References
- NIST DLMF, complex logarithm / inverse hyperbolic definitions. [DLMF].
- fdlibm / SunPro asinh implementation and modern libc sources (use copysign/special handling to avoid cancellation). [fdlibm / glibc sources].
- W. Kahan, “Branch cuts for complex elementary functions…”, discussion of sign bits and branch behaviour (commonly cited in implementations). [Kahan].
- Papers/benchmarks on Fortran/C library implementations and branch‑cut correctness (tests and implementation notes). [arXiv, library pages].
If you want, I can (a) show a tiny C/C++/Python code sample implementing the pseudocode above, or (b) fetch and paste the actual s_asinh.c implementation lines from fdlibm/glibc for direct comparison. Which would you prefer?
Simplify the conditional logic for computing r_re—both branches are mathematically equivalent.
The branching on z.im >= 0.0 is redundant:
let r_re = if z.im >= 0.0 {
m::copysign(m::log(m::hypot(z.re / 2.0, z.im / 2.0)) + M_LN2 * 2.0, z.re)
} else {
-m::copysign(m::log(m::hypot(z.re / 2.0, z.im / 2.0)) + M_LN2 * 2.0, -z.re)
};Both branches yield copysign(log_value, z.re). The else branch applies −copysign(value, −z.re), which simplifies via double negation to the same result as the if branch.
Replace with a single line:
let r_re = m::copysign(m::log(m::hypot(z.re / 2.0, z.im / 2.0)) + M_LN2 * 2.0, z.re);🤖 Prompt for AI Agents
In @src/cmath/trigonometric.rs around lines 254 - 266, The conditional creating
r_re in the large-argument branch is redundant because both branches compute the
same copysign(log(hypot(...)) + M_LN2*2.0, z.re); replace the if z.im >= 0.0 {
... } else { ... } block with a single assignment: set r_re using
m::copysign(m::log(m::hypot(z.re / 2.0, z.im / 2.0)) + M_LN2 * 2.0, z.re) and
remove the entire conditional; keep r_im and the return as-is.
| } else if z.re == 1.0 && ay < CM_SQRT_DBL_MIN { | ||
| // C99 standard says: atanh(1+/-0.) should be inf +/- 0i | ||
| if ay == 0.0 { | ||
| return Err(Error::EDOM); | ||
| } else { | ||
| let r_re = -m::log(m::sqrt(ay) / m::sqrt(m::hypot(ay, 2.0))); | ||
| let r_im = m::copysign(m::atan2(2.0, -ay) / 2.0, z.im); | ||
| return Ok(Complex64::new(r_re, r_im)); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add comments explaining the special case for z.re == 1.0.
Lines 313-322 implement a special case when z.re == 1.0 and ay < CM_SQRT_DBL_MIN with a non-obvious formula:
let r_re = -m::log(m::sqrt(ay) / m::sqrt(m::hypot(ay, 2.0)));
let r_im = m::copysign(m::atan2(2.0, -ay) / 2.0, z.im);This formula is difficult to verify without understanding the mathematical derivation.
Please add a comment explaining why this special case exists and the mathematical reasoning behind the formula, or provide a reference to the source (e.g., CPython's implementation).
} else if z.re == 1.0 && ay < CM_SQRT_DBL_MIN {
- // C99 standard says: atanh(1+/-0.) should be inf +/- 0i
+ // C99 standard says: atanh(1+/-0.) should be inf +/- 0i
+ // For z.re == 1.0 with very small ay, use special formula to maintain precision
+ // Derived from atanh(1 + iy) ≈ -log(sqrt(|y|/sqrt(|y|² + 4))) + i*atan2(2, -y)/2
if ay == 0.0 {🤖 Prompt for AI Agents
In @src/cmath/trigonometric.rs around lines 313 - 322, Add a clarifying comment
above the branch handling z.re == 1.0 && ay < CM_SQRT_DBL_MIN that explains this
is the C99/CPython special-case for atanh near the real argument 1 (to produce
+inf +/- 0i for ay==0 and a stable finite result for tiny positive ay), and
document the math: show that r_re = -log(sqrt(ay)/sqrt(hypot(ay,2))) is the
stable form of 0.5*log((1+|y|)/(1-|y|)) for tiny |y| and r_im =
copysign(atan2(2.0, -ay)/2.0, z.im) yields the correct sign and quadrant for the
imaginary part; include a short note pointing to CPython’s implementation or the
C99 reference as the source for this formula (mention CPython and C99 by name).
| #[cfg(target_os = "windows")] | ||
| { | ||
| unsafe extern "C" { | ||
| safe fn _errno() -> *mut i32; | ||
| } | ||
| *_errno() |
There was a problem hiding this comment.
Same safe fn syntax issue in get_errno.
The Windows branch has the same unusual unsafe extern "C" { safe fn _errno() } syntax as in set_errno. Please verify this compiles correctly on Windows.
🤖 Prompt for AI Agents
In @src/err.rs around lines 63 - 68, The Windows branch in get_errno uses the
invalid `safe fn _errno()` declaration; remove the `safe` keyword and declare
the external function as `extern "C" { fn _errno() -> *mut i32; }` and perform
the pointer access inside an unsafe block (mirroring the fix applied to
set_errno) so the code compiles on Windows and safely reads the errno value.
Summary by CodeRabbit
New Features
Tests
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.